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

cEP5: Define bear picking strategy and ambiguity on configuration #78

Merged
merged 2 commits into from
May 30, 2017

Conversation

adhikasp
Copy link
Member

@coala/aspects-developers

cEP-0005.md Outdated
@@ -3,7 +3,7 @@
| Metadata | |
| ------------ |-----------------------------------------------|
| cEP | 5 |
| Version | 2.0 |
| Version | 2.1 |
| Title | coala Configuration |
| Authors | Lasse Schuirmann <[email protected]> |

Choose a reason for hiding this comment

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

Line contains following spacing inconsistencies:

  • Trailing whitespaces.

SpaceConsistencyBear, severity NORMAL, section spacing.

The issue can be fixed by applying the following patch:

--- a/cEP-0005.md
+++ b/cEP-0005.md
@@ -88,7 +88,7 @@
 aspect name could be writen by its fullname (`Root.Redundancy.Clone`) or 
 partial name (`Redundancy.Clone` or even `Clone`) and are case insensitive. 
 Note that writing by partial name could result in ambiguity. Thus the desired
-aspect must be written by its fullname. 
+aspect must be written by its fullname.
 
 In case of multiple aspect has a same taste name, ambiguity will be resolved
 by prefixing the taste name with its aspect name. For example, `max_length`

Copy link
Member

@jayvdb jayvdb left a comment

Choose a reason for hiding this comment

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

The bear picking strategy should emit a log entry explaining the choice it made, and which bears were not used.

Copy link
Member

@userzimmermann userzimmermann left a comment

Choose a reason for hiding this comment

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

@adhikasp This is really nice :) Especially the prioritization! But there are some drawbacks in the texts...

cEP-0005.md Outdated
@@ -85,6 +85,16 @@ files = **.(c|h)
ignore_aspects = redundancy # Exclusive
```

aspect name could be writen by its fullname (`Root.Redundancy.Clone`) or
partial name (`Redundancy.Clone` or even `Clone`) and are case insensitive.
Copy link
Member

Choose a reason for hiding this comment

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

  • You start in singular (name, its) but end in plural (are)
  • Instead of fullname better write fully qualified name
  • Instead of partial name better write partially qualified name to avoid any misunderstandings
  • Better use can instead of could

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, grammar is never my best thing 😖

cEP-0005.md Outdated
aspect name could be writen by its fullname (`Root.Redundancy.Clone`) or
partial name (`Redundancy.Clone` or even `Clone`) and are case insensitive.
Note that writing by partial name could result in ambiguity. Thus the desired
aspect must be written by its fullname.
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to necessarily use the fully qualified name in that case. It must only be enough qualified to remove the ambiguity

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yeah, right

cEP-0005.md Outdated
In case of multiple aspect has a same taste name, ambiguity will be resolved
by prefixing the taste name with its aspect name. For example, `max_length`
is a taste under `Shortlog` and `LineLength`. Thus defining the taste must
be written by `Shortlog.max_length = 50`.
Copy link
Member

Choose a reason for hiding this comment

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

  • multiple aspects have ...
  • is a taste of ...
  • Thus the Shortlog taste would be defined like ...

@userzimmermann
Copy link
Member

Also, I agree with @jayvdb's #78 (review)

@adhikasp
Copy link
Member Author

@jayvdb

The bear picking strategy should emit a log entry explaining the choice it made, and which bears were not used.

Why we should tell which bears is not used, instead of just bear that is used?

@userzimmermann
Copy link
Member

userzimmermann commented May 30, 2017

@adhikasp Perfect 🎉 We only have to wait for @Techievena to rebase #72 , so that you can then rebase on his merged commit

And @jayvdb please approve as well :)

@userzimmermann
Copy link
Member

@adhikasp According to what I've written in https://gitlab.com/coala/GSoC-2017/issues/139#note_31058470 : Let's maybe better not wait for @Techievena to rebase #72 ... Just to hurry up regarding GSoC

cEP-0005.md Outdated
@@ -3,7 +3,7 @@
| Metadata | |
| ------------ |-----------------------------------------------|
| cEP | 5 |
| Version | 2.0 |
| Version | 2.1 |
| Title | coala Configuration |
| Authors | Lasse Schuirmann <[email protected]> |
Copy link
Member

Choose a reason for hiding this comment

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

add yourself as author

cEP-0005.md Outdated
@@ -85,6 +85,18 @@ files = **.(c|h)
ignore_aspects = redundancy # Exclusive
```

aspect name could be writen by its fully qualified name
Copy link
Member

Choose a reason for hiding this comment

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

Per #69 (comment) and prior comments, the cEP should not be forced to use lowercase aspects instead of using proper grammar, which is capitalised "Aspects" at the beginning of a sentence.

So, either use "Aspects" here, or adjust the sentence so that the word "aspects" is not the start of the sentence.

Copy link
Member

@userzimmermann userzimmermann May 30, 2017

Choose a reason for hiding this comment

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

@jayvdb
Copy link
Member

jayvdb commented May 30, 2017

ack d637e1c

@jayvdb
Copy link
Member

jayvdb commented May 30, 2017

@rultor merge

@rultor
Copy link

rultor commented May 30, 2017

@rultor merge

@jayvdb OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit d637e1c into coala:master May 30, 2017
@rultor
Copy link

rultor commented May 30, 2017

@rultor merge

@jayvdb Done! FYI, the full log is here (took me 2min)

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

Successfully merging this pull request may close these issues.

6 participants