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.

Sign up for GitHub

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

Jump to bottom

Clarify pipe naming requirement #31806

Closed
wants to merge 2 commits into from
Closed

Clarify pipe naming requirement #31806

wants to merge 2 commits into from

Conversation

jbogarthyde
Copy link
Contributor

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?

The naming requirement for pipes is only mentioned as a recommended naming convention in the style guide.

Issue Number: #31787

What is the new behavior?

Clarify requirement in style guide and add to the API doc for @Pipe decorator.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@jbogarthyde jbogarthyde requested review from a team as code owners July 23, 2019 18:32
@jbogarthyde jbogarthyde self-assigned this Jul 23, 2019
@ngbot ngbot bot added this to the Backlog milestone Jul 23, 2019
@mary-poppins
Copy link

You can preview 49553ee at https://pr31806-49553ee.ngbuilds.io/.

@jbogarthyde jbogarthyde added this to Committed - Selected for development in docs Jul 23, 2019
@jbogarthyde jbogarthyde moved this from Committed - Selected for development to In Progress in docs Jul 23, 2019
@jbogarthyde jbogarthyde changed the title docs: clarify pipe naming Clarify pipe naming requirement Jul 23, 2019
@santoshyadavdev
Copy link
Contributor

The doc related changes look good, but as @Airblader mentioned we should throw a proper exception from the framework.

Airblader
Airblader reviewed Jul 24, 2019
View reviewed changes
packages/core/src/metadata/directives.ts Outdated
* The pipe name to use in template bindings. Must be in
* [lowerCamelCase](guide/glossary#case-types), and by convention
* should match the class name. For example, the class `MyPipe`
* should use the name `myPipe`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree that something like a suffix "must" be kept. A NumberPipe would likely just have a name "number".

I also don't see why the name must match the class name in any way at all. There's no technical requirement for that.

The styleguide can be opionated, but IMHO here we should stick to facts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree - that's why the wording is "MUST be in lowerCamelCase" and "by convention SHOULD match class name". A one-word name that starts with lower case ("number") satisfies the requirement and does not throw an error.

Copy link
Contributor

Choose a reason for hiding this comment

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

But "must be in lowerCamelCase" is simply not true. "Cannot contain hyphens" is not the same as lowerCamelCase. I can also use rAndOmcASe if I want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That also passes the lowerCamelCase definition - starts with a lower case, followed by mixed case. Using uppercase for word-separation is a convention. However, I will add "cannot contain hyphens" - that is more specific, thanks.

@jbogarthyde
Copy link
Contributor Author

This PR is to satisfy the doc issue. There should be a separate issue for changing the code to throw a better exception.

@Airblader
Copy link
Contributor

Airblader commented Jul 24, 2019 •
edited

I can also use UpperCamelCase as a name. Again, there's no technical constraint for that.

@mary-poppins
Copy link

You can preview 92325a2 at https://pr31806-92325a2.ngbuilds.io/.

@mary-poppins
Copy link

You can preview a4a72cf at https://pr31806-a4a72cf.ngbuilds.io/.

@jbogarthyde jbogarthyde moved this from In Progress to Waiting for Approval in docs Jul 24, 2019
@jbogarthyde jbogarthyde force-pushed the jb-pipe-names branch 2 times, most recently from 663235a to 4cb698a Compare August 5, 2019 15:04
@jbogarthyde
Copy link
Contributor Author

@kara @igor Please check and approve.

@mary-poppins
Copy link

You can preview 663235a at https://pr31806-663235a.ngbuilds.io/.

@mary-poppins
Copy link

You can preview fb16673 at https://pr31806-fb16673.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 4cb698a at https://pr31806-4cb698a.ngbuilds.io/.

@jbogarthyde jbogarthyde removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Aug 5, 2019
@mary-poppins
Copy link

You can preview 06c60a9 at https://pr31806-06c60a9.ngbuilds.io/.

@mary-poppins
Copy link

You can preview ffee22a at https://pr31806-ffee22a.ngbuilds.io/.

@jbogarthyde
Copy link
Contributor Author

@kara I made your change, please check and approve.
@igor needs codeowner approval

kara
kara approved these changes Aug 8, 2019
View reviewed changes
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM

@kara kara 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 type: bug/fix labels Aug 8, 2019
@kara
Copy link
Contributor

kara commented Aug 8, 2019

merge-assistance: global approval

@kara kara closed this in 695f322 Aug 9, 2019
kara pushed a commit that referenced this pull request Aug 9, 2019
@jbogarthyde @kara
docs: clarify pipe naming (#31806)
3300331 
PR Close #31806
pkozlowski-opensource pushed a commit to pkozlowski-opensource/angular that referenced this pull request Aug 12, 2019
@jbogarthyde @pkozlowski-opensource
docs: clarify pipe naming (angular#31806)
cc8bb68 
PR Close angular#31806
@jbogarthyde jbogarthyde moved this from Waiting for Approval to Done in docs Aug 12, 2019
@jbogarthyde jbogarthyde mentioned this pull request Aug 12, 2019
Closed
@jbogarthyde jbogarthyde deleted the jb-pipe-names branch August 12, 2019 16:18
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
@jbogarthyde @sabeersulaiman
docs: clarify pipe naming (angular#31806)
690e96e 
PR Close angular#31806
@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 Sep 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Airblader Airblader Airblader left review comments

@trotyl trotyl trotyl left review comments

@kara kara kara approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker aio: preview cla: yes effort1: hours freq1: low merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note risk: low target: patch This PR is targeted for the next patch release
Projects
docs
Done
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jbogarthyde @mary-poppins @santoshyadavdev @Airblader @kara @trotyl @googlebot