Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(core): remove outdated comment #44099

Conversation

dario-piotrowicz
Copy link
Contributor

@dario-piotrowicz dario-piotrowicz commented Nov 6, 2021

remove the comment suggesting to use an enum for ViewEncapsulation as
the change seems to have already been applied in PR #25255

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: N/A

A code comment seems to be present but no longer valid.

Prior to PR #25255 the check for the encapsulation was done using a 2 and the comment suggested to substitute that with an enum, that seems to have been already done in the aforementioned PR

As you can see here:
Screenshot at 2021-11-06 17-11-54

What is the new behavior?

The comment has been removed

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

  • The comment also suggest that None should be 0 and not 2, is such a change still on the table?
    seems pretty risky and not too beneficial to me at this point, but since the enum is used maybe it could be fine? 🤷‍♂️
    anyways if that's the case I would suggest to put a comment for that in the enum itself
    (
    by the way, there's also a 1 not being used anymore there
    // Historically the 1 value was for `Native` encapsulation which has been removed as of v11.
    )

@google-cla google-cla bot added the cla: yes label Nov 6, 2021
@alan-agius4 alan-agius4 added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime target: patch This PR is targeted for the next patch release labels Nov 6, 2021
@ngbot ngbot bot modified the milestone: Backlog Nov 6, 2021
dario-piotrowicz added a commit to dario-piotrowicz/angular that referenced this pull request Nov 8, 2021
remove the comment suggesting to use a const enum for ViewEncapsulation in
the renderer3/definitions.ts file and add a similar comment in the
view.ts file in which the enum is defined

Note: the new comment does not contain the suggestion of changing `None`
to `0` as it is unclear what benefits that would bring (for more info
see: angular#44099 (comment))
@angular angular deleted a comment from mary-poppins Nov 8, 2021
@mary-poppins
Copy link

You can preview 3b7b1b3 at https://pr44099-3b7b1b3.ngbuilds.io/.

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

@dario-piotrowicz thanks for the cleanup 👍

packages/core/src/metadata/view.ts Outdated Show resolved Hide resolved
dario-piotrowicz added a commit to dario-piotrowicz/angular that referenced this pull request Nov 9, 2021
remove the comment suggesting to use a const enum for ViewEncapsulation in
the renderer3/definitions.ts file and add a similar comment in the
view.ts file in which the enum is defined

Note: the new comment does not contain the suggestion of changing `None`
to `0` as it is unclear what benefits that would bring (for more info
see: angular#44099 (comment))
@mary-poppins
Copy link

You can preview 67fb31a at https://pr44099-67fb31a.ngbuilds.io/.

remove the comment suggesting to use a const enum for ViewEncapsulation in
the renderer3/definitions.ts file and add a similar comment in the
view.ts file in which the enum is defined

Note: the new comment does not contain the suggestion of changing `None`
to `0` as it is unclear what benefits that would bring (for more info
see: angular#44099 (comment))
@dario-piotrowicz
Copy link
Contributor Author

@dario-piotrowicz thanks for moving the comment! Everything looks good, we just need to make sure that the updated files are formatted correctly (see the "lint" CI job output).

Sorry @AndrewKushnir I misread your comment and thought that we were waiting for some check, I didn't get that the check was failing 😅, I've fixed it, seems like the comment was too long so I broke it into two (0334d21)
(that's actually the automatic linter's fix)

I hope it's fine 🙂

@mary-poppins
Copy link

You can preview 0334d21 at https://pr44099-0334d21.ngbuilds.io/.

@AndrewKushnir AndrewKushnir removed the request for review from alan-agius4 November 10, 2021 19:26
@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 10, 2021
@AndrewKushnir
Copy link
Contributor

Note to the Caretaker: the legacy-unit-tests-saucelabs job is failing for unrelated reasons (timeouts). This change updates some comments, so it's safe to land.

@atscott
Copy link
Contributor

atscott commented Nov 10, 2021

This PR was merged into the repository by commit 0bd01ca.

atscott pushed a commit that referenced this pull request Nov 10, 2021
remove the comment suggesting to use a const enum for ViewEncapsulation in
the renderer3/definitions.ts file and add a similar comment in the
view.ts file in which the enum is defined

Note: the new comment does not contain the suggestion of changing `None`
to `0` as it is unclear what benefits that would bring (for more info
see: #44099 (comment))

PR Close #44099
@atscott atscott closed this in 0bd01ca Nov 10, 2021
dario-piotrowicz added a commit to dario-piotrowicz/angular that referenced this pull request Nov 10, 2021
slighlty improve the viewEncapsulation documentation (both in code
comments and md files) to make it more clear and understandable

as discussed here:
angular#44099 (comment)
dario-piotrowicz added a commit to dario-piotrowicz/angular that referenced this pull request Nov 10, 2021
slighlty improve the viewEncapsulation documentation (both in code
comments and md files) to make it more clear and understandable

as discussed here:
angular#44099 (comment)
@dario-piotrowicz dario-piotrowicz deleted the remove-viewEncapsulation-comment branch November 11, 2021 20:26
dario-piotrowicz added a commit to dario-piotrowicz/angular that referenced this pull request Nov 11, 2021
slighlty improve the viewEncapsulation documentation (both in code
comments and md files) to make it more clear and understandable

as discussed here:
angular#44099 (comment)
dario-piotrowicz added a commit to dario-piotrowicz/angular that referenced this pull request Nov 11, 2021
slighlty improve the viewEncapsulation documentation (both in code
comments and md files) to make it more clear and understandable

as discussed here:
angular#44099 (comment)
dario-piotrowicz added a commit to dario-piotrowicz/angular that referenced this pull request Nov 11, 2021
slighlty improve the viewEncapsulation documentation (both in code
comments and md files) to make it more clear and understandable

as discussed here:
angular#44099 (comment)
dario-piotrowicz added a commit to dario-piotrowicz/angular that referenced this pull request Nov 11, 2021
slighlty improve the viewEncapsulation documentation (both in code
comments and md files) to make it more clear and understandable

as discussed here:
angular#44099 (comment)
dario-piotrowicz added a commit to dario-piotrowicz/angular that referenced this pull request Nov 12, 2021
Slighlty improve the `viewEncapsulation` documentation (both in code
comments and content files) to make it more clear and understandable.

See angular#44099 (comment)
jessicajaniuk pushed a commit that referenced this pull request Nov 24, 2021
Slighlty improve the `viewEncapsulation` documentation (both in code
comments and content files) to make it more clear and understandable.

See #44099 (comment)

PR Close #44151
jessicajaniuk pushed a commit that referenced this pull request Nov 24, 2021
Slighlty improve the `viewEncapsulation` documentation (both in code
comments and content files) to make it more clear and understandable.

See #44099 (comment)

PR Close #44151
@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 Dec 12, 2021
dimakuba pushed a commit to dimakuba/angular that referenced this pull request Dec 28, 2021
remove the comment suggesting to use a const enum for ViewEncapsulation in
the renderer3/definitions.ts file and add a similar comment in the
view.ts file in which the enum is defined

Note: the new comment does not contain the suggestion of changing `None`
to `0` as it is unclear what benefits that would bring (for more info
see: angular#44099 (comment))

PR Close angular#44099
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 aio: preview area: core Issues related to the framework runtime cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants