-
Notifications
You must be signed in to change notification settings - Fork 1.5k
refactor Settings::premium #7932
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
Changes from all commits
e94598e
fc90808
d045d61
7755b02
950aa01
72e573d
d745686
8276f30
eecba32
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
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.
Great - now we have an additional way (again) to check for premium which fragments the code and needs to be removed again in the future...
Uh oh!
There was an error while loading. Please reload this page.
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.
imho it's more "fragmented" to have copy pasted code. If you want to refactor this in the future, feel free to do it but there will be compiler errors if you miss the refactoring somewhere. It will not be silently missed.
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.
This actually makes it more fragment and you also missed some cases. But I would prefer if you remove this change.
After moving the duplicated line you only had to add a single line to fix the tests (which was a bug but not yet exposed because there is nothing which could be tested yet). Everything else is just unnecessary cruft.
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 don't understand your goal and therefore it just seems to me that you rather have copy-pasted code. Can you share some insight into how it should be done instead?
I moved the
startsWith("Cppcheck Premium")to a single place: