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

addons/namingng.py: Fix commandline use. #5793

Merged
merged 2 commits into from
Dec 30, 2023
Merged

Conversation

mvds00
Copy link
Contributor

@mvds00 mvds00 commented Dec 22, 2023

namingng.py was only usable in standalone mode, but now supports CLI mode, i.e. with cppcheck --addon=namingng. It uses the generic reporting provided by cppcheckdata.reportError(). All output other than reported errors is suppressed.

A local function reportNamingError() is implemented to call through to cppcheckdata.reportError(), filling in common defaults.

The collection of errors and the --verify feature are removed, including related workflow and a test file. These are replaced by a unit test.

Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

thanks for giving this addon a bit of attention

addons/namingng.py Show resolved Hide resolved
@mvds00
Copy link
Contributor Author

mvds00 commented Dec 25, 2023

I got feedback in a different PR that a unit test should be implemented, so I did. To test namingng, the RE_FILE feature had to be fixed as well so this PR now contains a very minor fix (#5802 is an improved version of it).

@mvds00 mvds00 force-pushed the pullreq1 branch 3 times, most recently from 89bccef to e5e39b9 Compare December 25, 2023 01:53
Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

if you test the addon properly with test-other.py then I do not feel that the --verify option is needed and then we don't need to return errors from reportError..

namingng.py was only usable in standalone mode, but now supports CLI mode,
i.e. with cppcheck --addon=namingng. It uses the generic reporting provided
by cppcheckdata.reportError(). All output other than reported errors is
suppressed.

A local function reportNamingError() is implemented to call through to
cppcheckdata.reportError(), filling in common defaults.

The collection of errors and the --verify feature are removed, including
related workflow and a test file. These are replaced by a unit test, which now
also covers (include) file naming.
Confusingly, the example config file for namingng.py was named naming.json, while
another addon naming.py can be used using a *different* file that is also named
naming.json, residing in a different directory.

The feature where an addon can be named addon.json, referring with its "script"
key to the actual addon script, further adds to possible confusion.

By renaming the default namingng config file from naming.json to
namingng.config.json, it is made explicit that this file is a config file for
the namingng.py addon.

A file namingng.json is added as an example file to be used using
--addon=namingng.json.

This matches the setup of namingng as created and tested in test-other.py.
@mvds00
Copy link
Contributor Author

mvds00 commented Dec 27, 2023

if you test the addon properly with test-other.py then I do not feel that the --verify option is needed and then we don't need to return errors from reportError..

I did a cleanup and put all of it in one commit, as all changes are intertwined. The test case is not as elaborate as the --verify variant on some details, but does cover (include) file naming tests. I don't think every single violation of a pattern needs testing, rather just the concept of testing naming of filenames, function names and variable names, plus the prefix test feature. Please let me know if more tests should be added.

Furthermore, I added a commit from a different pending PR, regarding the naming of the default namingng config file. I also added a namingng.json example to that commit, to save new users the time to figure that one out.

@danmar danmar merged commit 24133d4 into danmar:main Dec 30, 2023
68 checks passed
@firewave
Copy link
Collaborator

firewave commented Jan 5, 2024

Thanks for working on this. I also ran into some of the issues with the addon and also filed https://trac.cppcheck.net/ticket/12005 about it not being working. I guess that ticket is now fixed.

I was considering to deprecate this addon as it was never maintained or tested at all after it was upstreamed (this applies to several addons). That's the reason I didn't post several changes I made to get it working. I am looking at them right now if there is still something to salvage.

As this addon is now getting more attention I wonder if we should deprecate the naming addon or replace it with the other one. Having two ways of doing the same thing doesn't make much sense.

@@ -446,8 +446,6 @@ jobs:
python3 ../naming.py --var='[a-z].*' --function='[a-z].*' naming_test.c.dump
../../cppcheck --dump naming_test.cpp
python3 ../naming.py --var='[a-z].*' --function='[a-z].*' naming_test.cpp.dump
../../cppcheck --dump namingng_test.c
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should at least keep a basic test to make sure it still works in standalone mode.

@mvds00
Copy link
Contributor Author

mvds00 commented Jan 5, 2024

As this addon is now getting more attention I wonder if we should deprecate the naming addon or replace it with the other one. Having two ways of doing the same thing doesn't make much sense.

I could simply implement --var=, --const=, --private-member-variable= and --function= in namingng.py (only for non-CLI mode) and it would be a drop-in replacement for naming.py, meaning we could then safely rename namingng.py to naming.py with nobody noticing.

update: #5846 implements the suggestion above

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

Successfully merging this pull request may close these issues.

3 participants