Skip to content

Conversation

ajitsinghkaler
Copy link
Contributor

Attribute decorator has defined attributeName as optional but actually its
mandatory and compiler throwa an error if attributeName is undefined. Made
attributeName mandatory in the Attribute decorator to reflect this functionality

Fixes #32658

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #32658

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

`Attribute` decorator has defined `attributeName` as optional but actually its
 mandatory and compiler throws an error if `attributeName` is undefined. Made
`attributeName` mandatory in the `Attribute` decorator to reflect this functionality

Fixes angular#32658
@ajitsinghkaler ajitsinghkaler marked this pull request as draft July 18, 2020 13:27
@ajitsinghkaler ajitsinghkaler removed the request for review from pkozlowski-opensource July 18, 2020 13:27
@ajitsinghkaler ajitsinghkaler marked this pull request as ready for review July 18, 2020 14:07
@sonukapoor
Copy link
Contributor

@ajitsinghkaler Are there any tests that cover this change?

@ajitsinghkaler
Copy link
Contributor Author

I don't think it is a breaking change because if the attributeName is left empty it will give you an error already. So eveybody should already be following it's just that we have not enforced it.

@ajitsinghkaler
Copy link
Contributor Author

@sonukapoor Actually I'm a bit confused if a test is actually needed for this because the attribute was already optional

I actually don't know even what actually to test for example should we test if the attributeName is empty it should throw but that was already happening. A bit confused by this

@sonukapoor
Copy link
Contributor

@sonukapoor Actually I'm a bit confused if a test is actually needed for this because the attribute was already optional

I actually don't know even what actually to test for example should we test if the attributeName is empty it should throw but that was already happening. A bit confused by this

I assume there is already a unit test for this, that checks if the attributeName is not passed, then an error is thrown. Can you just double-check for that?

@crisbeto
Copy link
Member

I don't know if that's actually the reason, but something to consider: maybe the idea with keeping the attributeName optional was that, if it omitted, the name would be picked up from the property that was decorated with it?

@ajitsinghkaler
Copy link
Contributor Author

ajitsinghkaler commented Jul 19, 2020

I don't think so even in docs it is not optional. Maybe not sure why but did not find anything like that

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM, if this throws an error already, I don't see this as a breaking change

Reviewed-for: public-api

@pullapprove pullapprove bot removed the request for review from IgorMinar July 23, 2020 03:33
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

LGTM!

Considering that there is no way to inject an attribute without also specifying the attribute name, this change is not a breaking change and therefor it should be fine to land it without much trouble.

If google3 presubmits pass, then we can go as far as landing this in a patch version.

Reviewed-for: public-api

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Reviewed-for: public-api

@ajitsinghkaler ajitsinghkaler added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jul 26, 2020
@jelbourn
Copy link
Member

@AndrewKushnir PTAL

@AndrewKushnir AndrewKushnir added area: compiler Issues related to `ngc`, Angular's template compiler area: core Issues related to the framework runtime type: bug/fix labels Jul 28, 2020
@ngbot ngbot bot modified the milestone: needsTriage Jul 28, 2020
@jelbourn jelbourn added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 28, 2020
@ajitsinghkaler
Copy link
Contributor Author

Should we add it to patch version

@mhevery mhevery added the target: patch This PR is targeted for the next patch release label Jul 28, 2020
@mhevery mhevery closed this in af80bdb Jul 28, 2020
mhevery pushed a commit that referenced this pull request Jul 28, 2020
`Attribute` decorator has defined `attributeName` as optional but actually its
 mandatory and compiler throws an error if `attributeName` is undefined. Made
`attributeName` mandatory in the `Attribute` decorator to reflect this functionality

Fixes #32658

PR Close #38131
Splaktar pushed a commit to angular-hispano/angular that referenced this pull request Aug 8, 2020
…r#38131)

`Attribute` decorator has defined `attributeName` as optional but actually its
 mandatory and compiler throws an error if `attributeName` is undefined. Made
`attributeName` mandatory in the `Attribute` decorator to reflect this functionality

Fixes angular#32658

PR Close angular#38131
@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 Aug 28, 2020
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
…r#38131)

`Attribute` decorator has defined `attributeName` as optional but actually its
 mandatory and compiler throws an error if `attributeName` is undefined. Made
`attributeName` mandatory in the `Attribute` decorator to reflect this functionality

Fixes angular#32658

PR Close angular#38131
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 area: compiler Issues related to `ngc`, Angular's template compiler area: core Issues related to the framework runtime cla: yes target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Attribute decorator should allow attribute name as optional
9 participants