Minimal Config for Testing #41

Closed
pq opened this Issue Mar 6, 2015 · 19 comments

Comments

Projects
None yet
4 participants
@pq
Member

pq commented Mar 6, 2015

Separate from the larger question of configurability in general (#7), we need a minimal config format for testing purposes. Whatever it ends up being, it should be labeled as provisional and consider it entirely subject to review and change.

Minimally, I'd like to see ways to:

  • include/exclude files
  • enable/disable specific rules

@pq pq added the enhancement label Mar 6, 2015

@pq

This comment has been minimized.

Show comment
Hide comment
@pq

pq Mar 6, 2015

Member

Here's a strawman, using YAML since we're already parsing it elsewhere and it should be familiar.

# File set description: lists of patterns to include/exclude
files:
  includes:
    - **/src
  excludes:
    - test/data/**

# Rule sets; ignore for now rule set inheritance and externally contributed rules
# Basic format:
#  [group]: 
#    [name]:  
#      ... config
rules:
  style_guide:
    UnnecessaryGetters: false
  pub:
    PubPackageNames:
      enabled: false # longer form of PubPackageNames: false

The above includes/excludes propose gitignore-style expressions. Nothing set in stone of course.

Comments very welcome.

@seaneagan? @zoechi?

Member

pq commented Mar 6, 2015

Here's a strawman, using YAML since we're already parsing it elsewhere and it should be familiar.

# File set description: lists of patterns to include/exclude
files:
  includes:
    - **/src
  excludes:
    - test/data/**

# Rule sets; ignore for now rule set inheritance and externally contributed rules
# Basic format:
#  [group]: 
#    [name]:  
#      ... config
rules:
  style_guide:
    UnnecessaryGetters: false
  pub:
    PubPackageNames:
      enabled: false # longer form of PubPackageNames: false

The above includes/excludes propose gitignore-style expressions. Nothing set in stone of course.

Comments very welcome.

@seaneagan? @zoechi?

@zoechi

This comment has been minimized.

Show comment
Hide comment
@zoechi

zoechi Mar 6, 2015

Can this already be tested or is this just a propisal?
A command line option to print all rule names would be helpful.

zoechi commented Mar 6, 2015

Can this already be tested or is this just a propisal?
A command line option to print all rule names would be helpful.

@pq

This comment has been minimized.

Show comment
Hide comment
@pq

pq Mar 6, 2015

Member

Just a rough idea as of yet. That said, if it looks reasonable, I'll go for it.

Member

pq commented Mar 6, 2015

Just a rough idea as of yet. That said, if it looks reasonable, I'll go for it.

@zoechi

This comment has been minimized.

Show comment
Hide comment
@zoechi

zoechi Mar 6, 2015

Yeah, looks fine.

zoechi commented Mar 6, 2015

Yeah, looks fine.

@seaneagan

This comment has been minimized.

Show comment
Hide comment
@seaneagan

seaneagan Mar 6, 2015

Looks like a great start!

I think the file set configuration is less urgent, since a package generally tries to adhere to a certain style throughout all its dart files, so the rule configuration gets you most of the way there. Having said that, I like include/exclude over includes/excludes and glob is probably sufficient (as opposed to gitignore style) which would match pub. I guess the default starting point if no include is given is all dart files? Might be nice to support include and exclude being a single glob instead of a list of globs.

I think snake_case rule identifiers may be more readable, since the rules may be phrase-like, e.g. unnecessary_getters.

If the rules are namespaced to a group, then the individual rules don't need to include the group name, so pub_package_names ==> package_names.

This would be e.g. a .dartlinter.yaml in the package root?

# File set description: lists of patterns to include/exclude
files:
  include: **/src
  exclude: test/data/**

# Rule sets; ignore for now rule set inheritance and externally contributed rules
# Basic format:
#  [group]: 
#    [name]:  
#      ... config
rules:
  style_guide:
    unnecessary_getters: false
  pub:
    package_names:
      enabled: false # longer form of package_names: false

Looks like a great start!

I think the file set configuration is less urgent, since a package generally tries to adhere to a certain style throughout all its dart files, so the rule configuration gets you most of the way there. Having said that, I like include/exclude over includes/excludes and glob is probably sufficient (as opposed to gitignore style) which would match pub. I guess the default starting point if no include is given is all dart files? Might be nice to support include and exclude being a single glob instead of a list of globs.

I think snake_case rule identifiers may be more readable, since the rules may be phrase-like, e.g. unnecessary_getters.

If the rules are namespaced to a group, then the individual rules don't need to include the group name, so pub_package_names ==> package_names.

This would be e.g. a .dartlinter.yaml in the package root?

# File set description: lists of patterns to include/exclude
files:
  include: **/src
  exclude: test/data/**

# Rule sets; ignore for now rule set inheritance and externally contributed rules
# Basic format:
#  [group]: 
#    [name]:  
#      ... config
rules:
  style_guide:
    unnecessary_getters: false
  pub:
    package_names:
      enabled: false # longer form of package_names: false
@pq

This comment has been minimized.

Show comment
Hide comment
@pq

pq Mar 6, 2015

Member

Thanks for the comments @seaneagan. A few follow-ups.

I like include/exclude over includes/excludes

Agreed! I started implementing this over lunch and that's what I chose too.

and glob is probably sufficient (as opposed to gitignore style) which would match pub.

👍

Might be nice to support include and exclude being a single glob instead of a list of globs.

How about we support both? That is:

  include: '**/src'

and, in case they want to specify more than one:

  include: 
    - '**/src'
   - '**/test'

Incidentally, we'll need to quote the globs since *'s interfere with YAML aliases. sigh

I think snake_case rule identifiers may be more readable, since the rules may be phrase-like, e.g. unnecessary_getters.

Neutral here. For some reason I was leaning towards CamelCase for rule names. Maybe so that the names correspond with the implementation classes? Dunno. At the moment, I'm using CamelCase in lint descriptions but that's definitely open to negotiation. Do you feel strongly?

If the rules are namespaced to a group, then the individual rules don't need to include the group name, so pub_package_names ==> package_names.

Agreed!

Member

pq commented Mar 6, 2015

Thanks for the comments @seaneagan. A few follow-ups.

I like include/exclude over includes/excludes

Agreed! I started implementing this over lunch and that's what I chose too.

and glob is probably sufficient (as opposed to gitignore style) which would match pub.

👍

Might be nice to support include and exclude being a single glob instead of a list of globs.

How about we support both? That is:

  include: '**/src'

and, in case they want to specify more than one:

  include: 
    - '**/src'
   - '**/test'

Incidentally, we'll need to quote the globs since *'s interfere with YAML aliases. sigh

I think snake_case rule identifiers may be more readable, since the rules may be phrase-like, e.g. unnecessary_getters.

Neutral here. For some reason I was leaning towards CamelCase for rule names. Maybe so that the names correspond with the implementation classes? Dunno. At the moment, I'm using CamelCase in lint descriptions but that's definitely open to negotiation. Do you feel strongly?

If the rules are namespaced to a group, then the individual rules don't need to include the group name, so pub_package_names ==> package_names.

Agreed!

@zoechi

This comment has been minimized.

Show comment
Hide comment
@zoechi

zoechi Mar 6, 2015

I 'm currently working on a project where I use a lot of code generation. I might want to use different sets of rules for the generated files.

zoechi commented Mar 6, 2015

I 'm currently working on a project where I use a lot of code generation. I might want to use different sets of rules for the generated files.

@seaneagan

This comment has been minimized.

Show comment
Hide comment
@seaneagan

seaneagan Mar 6, 2015

@zoechi Agreed, one does care less about style in generated than hand-written code. It should be pretty easy to implement anyways considering the glob package availability.

@zoechi Agreed, one does care less about style in generated than hand-written code. It should be pretty easy to implement anyways considering the glob package availability.

@seaneagan

This comment has been minimized.

Show comment
Hide comment
@seaneagan

seaneagan Mar 6, 2015

@pq Yep, supporting glob and list-of-glob would be ideal.

I assumed having the rules ids matching the dart class names was a motiviation, but for me that's actually a negative since it feels like the impl leaking into the user interface. I looked at the JS linters:

They all agree on starting with a lower case letter. I actually think eslint's lowerCamelCase looks fine, and is easier to type than UpperCamelCase. That'd be a close second to snake_case for me.

@pq Yep, supporting glob and list-of-glob would be ideal.

I assumed having the rules ids matching the dart class names was a motiviation, but for me that's actually a negative since it feels like the impl leaking into the user interface. I looked at the JS linters:

They all agree on starting with a lower case letter. I actually think eslint's lowerCamelCase looks fine, and is easier to type than UpperCamelCase. That'd be a close second to snake_case for me.

@a14n

This comment has been minimized.

Show comment
Hide comment
@a14n

a14n Mar 7, 2015

Contributor

About code gen (it's a trending topic :)) with source_gen I use a pattern where a private class is used as template to generate a public class and I'd like to be able to avoid warnings such like This private class is never used by excluding a particular class in a library and not a file or the library.

So it would be great to have a global exclusion for classes implementing something or annotated with something.

Contributor

a14n commented Mar 7, 2015

About code gen (it's a trending topic :)) with source_gen I use a pattern where a private class is used as template to generate a public class and I'd like to be able to avoid warnings such like This private class is never used by excluding a particular class in a library and not a file or the library.

So it would be great to have a global exclusion for classes implementing something or annotated with something.

@pq

This comment has been minimized.

Show comment
Hide comment
@pq

pq Mar 7, 2015

Member

@14n: thanks for chiming in! Can you add a link to the generator you're using? I'd be interested in taking a look. (EDIT: re-reading, I gather you mean source_gen.)

@seaneagan : great points. Thanks for the links to prior art. I don't see any strong reason not to go with underscores. Thanks again for all the back and forth. Keep it coming! :)

Member

pq commented Mar 7, 2015

@14n: thanks for chiming in! Can you add a link to the generator you're using? I'd be interested in taking a look. (EDIT: re-reading, I gather you mean source_gen.)

@seaneagan : great points. Thanks for the links to prior art. I don't see any strong reason not to go with underscores. Thanks again for all the back and forth. Keep it coming! :)

@pq

This comment has been minimized.

Show comment
Hide comment
@pq

pq Mar 7, 2015

Member

As far as annotations in source, I'm guessing we won't use metadata proper but will use some kind of comment convention. Open to ideas though.

Member

pq commented Mar 7, 2015

As far as annotations in source, I'm guessing we won't use metadata proper but will use some kind of comment convention. Open to ideas though.

@a14n

This comment has been minimized.

Show comment
Hide comment
@a14n

a14n Mar 7, 2015

Contributor

@pq yes I use source_gen to try to improve js-interop ( /cc @kevmoo ). I'm still looking for the good/best pattern to used and for now I use private classes as templates. For instance see the unused private class template used to generate the real public class. With the last version of the editor I get an hint:

The class '_JsFoo' is not used

I'd really like to have a way to disable this hint on every classes annotated with @JsProxy.

Contributor

a14n commented Mar 7, 2015

@pq yes I use source_gen to try to improve js-interop ( /cc @kevmoo ). I'm still looking for the good/best pattern to used and for now I use private classes as templates. For instance see the unused private class template used to generate the real public class. With the last version of the editor I get an hint:

The class '_JsFoo' is not used

I'd really like to have a way to disable this hint on every classes annotated with @JsProxy.

@pq

This comment has been minimized.

Show comment
Hide comment
@pq

pq Mar 8, 2015

Member

Ah, thanks for the clarification @a14n. Put that way, the issue really transcends the linter and is really about configuring the analyzer (cc @bwilkerson) since it's the source of the suspect hint. That said, we want to solve it consistently for lints and hints (and possibly warnings too) and I'm keen to push on that. I suspect there may well soon enough be lints you'll want to similarly disable in the context of source_gen in any case. What's interesting here is that you really want to plug a kind of error filter into the analyzer rather than a source annotation. Clearly file-level globs won't cut it.

Member

pq commented Mar 8, 2015

Ah, thanks for the clarification @a14n. Put that way, the issue really transcends the linter and is really about configuring the analyzer (cc @bwilkerson) since it's the source of the suspect hint. That said, we want to solve it consistently for lints and hints (and possibly warnings too) and I'm keen to push on that. I suspect there may well soon enough be lints you'll want to similarly disable in the context of source_gen in any case. What's interesting here is that you really want to plug a kind of error filter into the analyzer rather than a source annotation. Clearly file-level globs won't cut it.

@pq pq self-assigned this Mar 8, 2015

@pq

This comment has been minimized.

Show comment
Hide comment
@pq

pq Mar 10, 2015

Member

Support for the provisional format is landed. Sample here. For now, configs are specified on the command line using the -c option. We can settle on a default location down the road.

Member

pq commented Mar 10, 2015

Support for the provisional format is landed. Sample here. For now, configs are specified on the command line using the -c option. We can settle on a default location down the road.

@zoechi

This comment has been minimized.

Show comment
Hide comment
@zoechi

zoechi Mar 16, 2015

Works fine so far 👍

zoechi commented Mar 16, 2015

Works fine so far 👍

@pq

This comment has been minimized.

Show comment
Hide comment
@pq

pq Jun 15, 2015

Member

Bumping this old conversation as I'm doing a bit of a revisit on the way to tightening the integration with the analyzer.

One thing I think I'd like to change for sure is the need to specify a group. IOW (assuming no name collisions), the following would be equivalent:

rules:
  style_guide:
    unnecessary_getters: false
    camel_case_types: true
rules:
  unnecessary_getters: false
  camel_case_types: true

Sound reasonable?

@zoechi : any more feedback?

Member

pq commented Jun 15, 2015

Bumping this old conversation as I'm doing a bit of a revisit on the way to tightening the integration with the analyzer.

One thing I think I'd like to change for sure is the need to specify a group. IOW (assuming no name collisions), the following would be equivalent:

rules:
  style_guide:
    unnecessary_getters: false
    camel_case_types: true
rules:
  unnecessary_getters: false
  camel_case_types: true

Sound reasonable?

@zoechi : any more feedback?

@zoechi

This comment has been minimized.

Show comment
Hide comment
@zoechi

zoechi Jun 15, 2015

Verbosity doesn't bother me much, I just want to be able to express my requirements. Others often have a contrary opinion so they might love it :)

I'd love to have more checks ;-) A check for imports from transitive dependencies #29 would have saved me quite some time today (caused weird crashes dart-lang/pub#1114).

zoechi commented Jun 15, 2015

Verbosity doesn't bother me much, I just want to be able to express my requirements. Others often have a contrary opinion so they might love it :)

I'd love to have more checks ;-) A check for imports from transitive dependencies #29 would have saved me quite some time today (caused weird crashes dart-lang/pub#1114).

@pq

This comment has been minimized.

Show comment
Hide comment
@pq

pq Aug 17, 2016

Member

Closing for staleness.

Member

pq commented Aug 17, 2016

Closing for staleness.

@pq pq closed this Aug 17, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment