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

Library prefixes should allow starting with _ #684

Closed
srawlins opened this issue May 26, 2017 · 13 comments · Fixed by #770
Closed

Library prefixes should allow starting with _ #684

srawlins opened this issue May 26, 2017 · 13 comments · Fixed by #770
Labels
type-enhancement A request for a change that isn't a bug

Comments

@srawlins
Copy link
Member

Related to #294

import 'a.dart' as _i1;

violates library_prefixes with "Use lowercase_with_underscores when specifying a library prefix." Buuuut it looks good to me. ☹️

@srawlins
Copy link
Member Author

The current real world use case is in code_builder.

@srawlins
Copy link
Member Author

CC @matanlurey

@pq pq added the type-enhancement A request for a change that isn't a bug label Jun 8, 2017
@pq
Copy link
Member

pq commented Jun 22, 2017

@munificent : this looks reasonable to me; curious if you have any thoughts?

@munificent
Copy link
Member

It doesn't violate the style guide, so it's fine with me. It is a little weird to use a leading underscore for an identifier where privacy rules don't come into play. 😕

@pq
Copy link
Member

pq commented Jun 22, 2017

Thanks @munificent!

It is a little weird to use a leading underscore for an identifier where privacy rules don't come into play.

I hear you. I think the idea is to communicate a "this is an internal detail" intent (and avoid possible collisions). @matanlurey and @srawlins can edify though.

@natebosch
Copy link
Member

What will it take to change this? Looks like we need to tweak the regex here?
https://github.com/dart-lang/sdk/blob/1d7d954bf86b7d2cc49faeb016ddb0aca72cf46d/pkg/analyzer/lib/src/lint/util.dart#L21

@pq
Copy link
Member

pq commented Aug 3, 2017

I'll take a look at this today. 👍

@bwilkerson
Copy link
Member

Possibly. We'd need to see what other rules that change would effect, though. We might need to add a second regex that allows underscore and fix the code so that the new regex is used where appropriate.

@pq
Copy link
Member

pq commented Aug 3, 2017

@bwilkerson : I've looked into it and it's not bad. I've got an approach in mind. Stay tuned! 😄

@natebosch
Copy link
Member

Has this regressed? 2.0.0-dev.6.0 is reporting this lint for prefixes like _1 or _i1

@pq
Copy link
Member

pq commented Nov 10, 2017

Hey @natebosch!

It looks like in HEAD,_i1 passes but _1 does not. That said, I'm not sure that _1 was ever supported. Could you confirm that _i1 works for you? Do you feel strongly that _1 should pass?

@natebosch
Copy link
Member

I thought I tested _i1 and with dev-3.0 it was still a lint.

Testing now with dev-6.0 I'm not seeing a lint. Either it's been fixed in between or I was holding it wrong when I was testing last night.

I think it's fine that _1 produces a lint.

natebosch added a commit to dart-lang/code_builder that referenced this issue Nov 10, 2017
This matches old behavior and is a little more clear than just a digit.
This will also satisfy the lint on the latest SDK. See
dart-lang/linter#684 (comment)
@pq
Copy link
Member

pq commented Nov 10, 2017

Excellent. Thanks for following up! 👍

pq added a commit that referenced this issue Nov 10, 2017
pq added a commit that referenced this issue Nov 10, 2017
* Regression tests for `library_prefixes` (#684).

See: #684.

* sorted
natebosch added a commit to dart-lang/code_builder that referenced this issue Nov 14, 2017
This matches old behavior and is a little more clear than just a digit.
This will also satisfy the lint on the latest SDK. See
dart-lang/linter#684 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants