refactor Settings::premium #7932
Conversation
|
I told you there were failing tests... |
yes I wanted to look at that. |
|
I do not think it is important that a single line is currently duplicated. Also as mentioned before I have other changes coming up around the premium stuff so any changes might conflict or become obsolete. I am currently focused on the unmatched suppression stuff and I have other stuff (MinGW, CMake, ...) to look at first before I get around to finish up the code to post a PR. |
in my humble opinion I dislike to have duplicated code. why would it cause "conflicts" yeah the code might have to be changed but you are going to remove the duplication anyway right? If we have duplication there is a risk that it will be forgotten eventually and will remain. How can you proove that when you will refactor that you will not forget some duplication? |
| @@ -5,39 +5,46 @@ | |||
| <name>About</name> | |||
| <message> | |||
| <location filename="about.ui" line="14"/> | |||
| <location filename="build-cppcheck-Desktop-Debug/gui/ui_about.h" line="130"/> | |||
There was a problem hiding this comment.
@firewave this is wrong. the filename should not be "build-cppcheck-Desktop-Debug/gui/ui_about.h". If you build the GUI on the command line in a "build" folder I bet you get different paths. In my opinion these ts changes should be reverted. Please cleanup these changes of the ts files or else we remove that ts files are generated automatically. I don't want to keep cleaning up the mess manually anymore.
There was a problem hiding this comment.
See #7020 - I am still waiting on (external) feedback on that.
There was a problem hiding this comment.
We also said that we don't commit those files before a release.
There was a problem hiding this comment.
You did still not drop these changes.
There was a problem hiding this comment.
Your comment, “You did still not drop these changes,” came across to me as harsh. I’ve been against generating the TS files automatically because I anticipated it would disrupt our workflow, and I’m tired of repeatedly cleaning them up. If the cleanup isn’t handled moving forward, we will need to disable the TS file generation.
Geez. The PR was just merged two days ago after it had been waiting for feedback for months. And I said that I will look into it. And it is just a single line and that is suddenly the highest priority?!?!? If you want "proof" I can swamp the repo with PRs which contain intended changes which are hacks and/or incomplete. In this case - see https://github.com/firewave/cppcheck/tree/cfg-name for just some basic changes I threw together. |
| static bool isCppcheckPremium(const std::string& productName); | ||
|
|
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
which fragments the code and needs to be removed again in the future...
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.
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.
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:
- if the string literal has a typo in 1 place the code will compile fine and we don't have great testing of Cppcheck GUI so it might not be spotted.
- when it is refactored later I fear that you will miss some places. If I missed a place then why wouldn't you.. so it feels safer to avoid the copy/paste immediately.
|
In that case you also need to get rid of the productname being duplicated in the GUI and the settings otherwise this PR would be incomplete. But that requires other changes ... and other changes. And then you get into how we treat the version wrong etc. That's what my changes are about - it will just be incremental since it affects external stuff (i.e. premium). Paul wants his CMake stuff in (which requires review), you want a MinGW release (which requires lots of changes) and now discussions about a single duplicated line. And all of it is urgent (which it really isn't). And I am actually working on something which allows us to enable something which has never worked since this project existed and has been years in the making. I do not understand these priorities ... This should just be a three line change. Everything else will be immediately removed when I post my PR. |
I believe my MinGW release binary was quite important, as I’ve observed up to 3x speed improvements. If we can be fast it's a strong USP. There is a quite a lot of flux in Cppcheck code to just refactor it. And well it does introduce bugs. We have several user problems—usability concerns, false positives, and so on—that I would personally prioritize investigating. That said, I understand that we all have our own areas of interest and personal “itches” we want to address.
sounds good of course.. |
It is my goal that you remove everything else later when you refactor it. That is not a problem. The problem is if we miss to refactor some place.. |
2 participants
No description provided.