Skip to content

Conversation

rafaelss95
Copy link
Contributor

Basically the same idea of PR #9586, which fixed #9582, but unfortunately didn't add a TSLint rule to keep these changes consistent.

Now, I'm adding Codelyzer's prefer-output-readonly rule to the tslint.json :).

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 11, 2019
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels redundant IMO. It makes us write more code while enforcing a rule that hasn't been an issue until now.

@rafaelss95
Copy link
Contributor Author

This feels redundant IMO. It makes us write more code while enforcing a rule that hasn't been an issue until now.

Hmm... I thought it was an issue since #9582 was created explicitly to do this. The problem is that in the PR abovementioned I didn't add a rule to keep it consistent. That said, should I close this then?

@crisbeto
Copy link
Member

I think that @jelbourn can say whether that issue is still relevant since it's very old.

@jelbourn
Copy link
Member

I do think we should still do this. It may be a bit of extra effort, but it is more correct.

@rafaelss95 rafaelss95 force-pushed the refactor/readonly branch 6 times, most recently from 5b762a5 to 6907f5b Compare August 9, 2020 06:47
@jelbourn jelbourn added the target: major This PR is targeted for the next major release label Aug 10, 2020
@jelbourn jelbourn added this to the 11.0.0 milestone Aug 10, 2020
@rafaelss95 rafaelss95 force-pushed the refactor/readonly branch 4 times, most recently from c0ace39 to 07bbceb Compare August 22, 2020 22:12
@rafaelss95 rafaelss95 force-pushed the refactor/readonly branch 6 times, most recently from e771d6b to d06bec6 Compare March 18, 2021 02:30
@mmalerba mmalerba added the action: merge The PR is ready for merge by the caretaker label Mar 18, 2021
Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mmalerba mmalerba added the merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed label Mar 18, 2021
@rafaelss95
Copy link
Contributor Author

@mmalerba, could you please suggest a commit message to make lint happy? :)

@crisbeto
Copy link
Member

You have to add a scope to it, e.g. refactor(<scope>): make all event emitters readonly. These are the possible scopes: https://github.com/angular/components/blob/master/.ng-dev/commit-message.ts#L10

@rafaelss95
Copy link
Contributor Author

@crisbeto Thanks for the link! I looked at it and I couldn't find a suitable one, as the changes are being applied to several places at the same time.

@mmalerba mmalerba added the P2 The issue is important to a large percentage of users, with a workaround label Mar 29, 2021
@mmalerba
Copy link
Contributor

Yeah our commit message format doesn't work well for changes like this. You can just pick one, material/core maybe

@annieyw
Copy link
Contributor

annieyw commented Apr 1, 2021

@rafaelss95 please rebase/fixup message when you get the chance

@rafaelss95 rafaelss95 force-pushed the refactor/readonly branch 3 times, most recently from c1c6eeb to 3be6170 Compare April 3, 2021 03:15
@rafaelss95
Copy link
Contributor Author

@mmalerba @annieyw sorry for the delay, I rebased and fixed up the commit message.

@annieyw annieyw removed the merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed label Apr 5, 2021
@mmalerba mmalerba force-pushed the refactor/readonly branch from 3be6170 to 46aa4bb Compare April 7, 2021 05:25
@mmalerba mmalerba force-pushed the refactor/readonly branch from 46aa4bb to 5d4545a Compare April 7, 2021 05:42
@annieyw annieyw merged commit 4a3ca82 into angular:master Apr 7, 2021
@rafaelss95 rafaelss95 deleted the refactor/readonly branch April 7, 2021 10:01
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P2 The issue is important to a large percentage of users, with a workaround target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants