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.

Sign up for GitHub

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

Jump to bottom

addons/namingng.py: Backward compatibility with addons/naming.py #5846

Closed
wants to merge 1 commit into from
Closed

addons/namingng.py: Backward compatibility with addons/naming.py #5846

wants to merge 1 commit into from

Conversation

mvds00
Copy link
Contributor

@mvds00 mvds00 commented Jan 5, 2024

In case naming.py cannot be simply removed, due to it being in active use, this PR suggests a path forward, merging namingng.py and naming.py, with little risk.

A next step could be to drop namingng.py and s/namingng/naming/g throughout the project.

@mvds00 mvds00 mentioned this pull request Jan 5, 2024
Merged
@mvds00 mvds00 force-pushed the pullreq12 branch 2 times, most recently from a64fdc2 to 87bfe78 Compare January 5, 2024 23:50
@mvds00
Copy link
Contributor Author

mvds00 commented Jan 6, 2024

Note that the failing check is due to the issue fixed by #5847

@danmar
Copy link
Owner

danmar commented Jan 6, 2024

Note that the failing check is due to the issue fixed by #5847

these errors does not look related to #5847

https://github.com/danmar/cppcheck/actions/runs/7427881828/job/20227959060?pr=5846

@mvds00
Copy link
Contributor Author

mvds00 commented Jan 6, 2024 •
edited
Loading

Note that the failing check is due to the issue fixed by #5847

these errors does not look related to #5847

https://github.com/danmar/cppcheck/actions/runs/7427881828/job/20227959060?pr=5846

You are right. The suppressions don't work anymore because the error id [naming-varname] in the output is now [naming-namingConvention]. Brutal. What do you prefer: should I update .selfcheck_suppressions to suppress the new error id, or should I add another compatibility feature to namingng.py to make it more closely act like naming.py, even matching the error ids?

Or maybe you could first answer the bigger question: what do you think about this naming/namingng development, and which direction would you like it to go? What behaviors/interfaces are important to keep and what can we safely change?

@mvds00
addons/namingng.py: Implement backward compatibility with addons/nami…
1f67073 
…ng.py.

Merging naming.py into namingng.py reduces redundant code and leverages
argument validation and the unit tests for namingng.py, with the aim to improve
maintainability.

naming.py supported RE_CONST to check constants, which is now a feature of
namingng.py as well; the unit test is expanded to cover the feature.

naming.py has been updated to call through to namingng.py.

Suppressions are updated to suppres the new namingng error id
[naming-namingConvention].

The unit tests for naming.py are kept as-is.
@mvds00
Copy link
Contributor Author

mvds00 commented Jan 7, 2024

Or maybe you could first answer the bigger question: what do you think about this naming/namingng development, and which direction would you like it to go? What behaviors/interfaces are important to keep and what can we safely change?

Not all issues were solved with suppressions. I've been trying to make namingng.py behave like naming.py, but their approaches are hard to unify. naming.py considers const-ness in a way that namingng.py cannot do now. Going forward I'm thinking of adding a new set of configuration options to namingng.py, that would allow it to be configured in a naming.py-compatible way.

@mvds00
Copy link
Contributor Author

mvds00 commented Jan 8, 2024

This PR will become part of #5852

@mvds00 mvds00 closed this Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mvds00 @danmar