This repository was archived by the owner on Nov 3, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduce convention class #589
Introduce convention class #589
Changes from 4 commits
801fc92
6d3a59d
9a54a2a
8e9ef40
0a393a2
0c9ba5b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if there is a common subset of the errors being removed from each of the conventions that makes sense to name. E.g., something along the lines of "very strict rules".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't love the "all_errors - {subset}" logic, but not just because of code duplication. Any time anyone adds a new convention, we have to add it to this list. Personally, it makes more sense to me to make these additive instead of subtractive. But let's save this for another issue/PR since this PR doesn't change anything, just moves it around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the advantage of using a class instead of just using an Enum and keeping the AttrDict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea was to eventually have the convention class itself check the code and select the appropriate checker function. You can probably also just throw the name of the convention into the convention checker and that's it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second part seems especially useful to me. Going very far down the road to DWIM ("do what I mean") gets pretty painful. Users end up having to trick the tool into doing what they want instead of just specifying it as this approach allows.