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

Check for named constants #86

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Check for named constants #86

wants to merge 17 commits into from

Conversation

exussum12
Copy link
Collaborator

Eg remove $six = 6

or $week = 7

These sorts of numbers are still magic, fixes #83

exussum12 and others added 3 commits January 28, 2019 06:41
@exussum12
Check for named constants
ebd6a7e 
Eg remove $six = 6

or $week = 7

These sorts of numbers are still magic, fixes povils#83
@exussum12
PSR-2
0862546
@exussum12
PSR-2
ee2fa2c
@exussum12
Copy link
Collaborator Author

Not sure why appveyor is complaining. Will take a look later. Passes travis

@exussum12 exussum12 mentioned this pull request Mar 5, 2019
Merged
@exussum12
Copy link
Collaborator Author

This should be multilingual now.

@ramonacat
Copy link

Is the complexity really worth the benefit? I mean, you can still call your constant WHATEVER and the tool won't complain. You can still do const FIRST_COLOUR=1 and it'll pass fine.
I don't think this tool can ever replace thorough code review, it can help, by ensuring there are constants for everything, but IMO deciding if the name of the constant is meaningful enough cannot be really automated.

@exussum12
Copy link
Collaborator Author

I don't think this tool can ever replace thorough code review, it can help, by ensuring there are constants for everything

Completely agree. Since using this Ive seen more and more bad constant naming. That can not be prevented but the main cases I have seen, this will prevent that.

To be clear this check is not enabled by default, so its only if you want the additional level of checking.

FIRST_COLOUR=1 may be fine, because FIRST_COLOUR=2 may also be valid.

SIX=6 is never valid.

@exussum12
Copy link
Collaborator Author

@sidz thoughts on getting this in soon ? It will solve lots of magic constants. I don't think it should be Included in all but as a separate flag to not suddenly include lots of magic numbers

@sidz
Copy link
Collaborator

sidz commented Dec 12, 2021

As an idea it's ok for me so far. But we need to rebase this PR against latest master and fix all conflicts first.

@exussum12
Copy link
Collaborator Author

Yeah was just checking that before I did that work. I'll sort the conflicts this week

@exussum12 exussum12 added this to the 3.0 milestone Dec 13, 2021
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
3.0
Development

Successfully merging this pull request may close these issues.

Check names for numeric values
3 participants
@exussum12 @ramonacat @sidz