Allow `$` in casing-related rules #453

ernestognw · 2023-07-14T21:08:21Z

Fixes #452

I acknowledge the issue requires a bit more discussion but I'm opening this PR in case there's no push back for this rule change.

Alternatively, we can pass it as part of the rule configuration, similar to:

{
  "rules": {
    "var-name-mixedcase": ["error", { ignore$: true }]
  }
}

dbale-altoros · 2023-07-17T20:07:02Z

@ernestognw Thanks a lot for your input
The thing with this is that the isMixedCase() function is being called by:
lib/rules/naming/func-param-name-mixedcase.js
lib/rules/naming/func-name-mixedcase.js
lib/rules/naming/modifier-name-mixedcase.js
lib/rules/naming/var-name-mixedcase.js

So we need to make sure it doesn't break any other ruleset

Also, as I understand in the lib/common/identifier-naming.js there are
isCamelCase()
isNotCamelCase()
isUpperSnakeCase()
isNotUpperSnakeCase()

and (I'm not sure) but I think $ can also be present in function names, constants and parameter so, I'm thinking that if we add this, we should also add in those as well (?) just thinking out loud

ernestognw · 2023-07-17T21:50:46Z

Hey @dbale-altoros,

Yes, I think generally the mixed case should not fail if there's a $ at the beginning of its definition. Note that the rule already allows $ in between. So perhaps the discussion is if we should allow it at the beginning (or standalone).

I would say that allowing only standalone $ would be the less breaking change but the regex is already half of the way by allowing it after the first character.

I can update the other rules if you think it's the way to move forward

dbale-altoros · 2023-07-18T14:45:12Z

yes, the main thing is the leading $

I'm thinking if this really worth the hassle...
I understand that solhint shouldn't see that as a warning/error since it's allowed. That's for sure and we appreciate your input. Really!

If you are up to check in the other rules and add tests to make sure this won't break, we can move forward... if not, we can wait on this fix since doesn't seem to be a high priority

What do you think ?

ernestognw · 2023-07-18T19:07:52Z

Thanks also for your prompt response! Sure I think I can take a look at other rules and add tests.

Just confirming, should I look only for rules using the following functions?

isCamelCase()
isNotCamelCase()
isUpperSnakeCase()
isNotUpperSnakeCase()

dbale-altoros · 2023-07-19T13:11:57Z

yes, I think so
because in your pr the pattern being changed is only in that file with those functions

dbale-altoros · 2023-07-19T13:28:37Z

I also changed the base from master into develop
please put it as draft until it is ready

thanks a lot for your contribution and time... we appreciate!!!

ernestognw · 2023-07-19T20:45:13Z

Thanks for the guidance @dbale-altoros, I just marked this as ready for review since I already added tests for the rules affected. Should be enough but please let me know if there's something I'm missing.

dbale-altoros · 2023-07-20T13:49:05Z

of course... this week or next it will be reviewed by team
Thanks a lot for your contribution

dbale-altoros · 2023-08-01T13:45:46Z

@ernestognw sorry this is taking so long
we made a new rule called immutable-vars-naming changing some behavior on const-name-snakecase and var-name-mixed-case

So this pr has a few conflicts, I don't want to mess it up
I'll be reviewing it today or tomorrow

dbale-altoros · 2023-08-01T15:06:15Z

test/rules/naming/const-name-snakecase.js

+    }
+
+    for (const [key, code] of Object.entries(WITH_$)) {
+      it(`should not raise event name error for events ${key}`, () => {


seems this description is not ok, right ?
Don't worry I'll fix... just wanted to make sure

You're correct, it had to be:

Suggested change

it(`should not raise event name error for events ${key}`, () => {

it(`should not raise event name error for constants ${key}`, () => {

dbale-altoros · 2023-08-01T15:19:25Z

test/rules/naming/event-name-camelcase.js

+    for (const [key, code] of Object.entries(WITH_$)) {
+      it(`should not raise event name error for events ${key}`, () => {
+        const report = linter.processStr(code, {
+          rules: { 'contract-name-camelcase': 'error' },


this one should be event-name-camelcase, right ? (I fix)

Correct, it should be:

Suggested change

rules: { 'contract-name-camelcase': 'error' },

rules: { 'event-name-camelcase': 'error' },

dbale-altoros · 2023-08-01T15:27:28Z

test/rules/naming/func-param-name-mixedcase.js

+      }
+
+      for (const [key, code] of Object.entries(WITH_$)) {
+        it(`should not raise func name error for functions ${key}`, () => {


for Event Parameters
(wrong description on IT statement)

Suggested change

it(`should not raise func name error for functions ${key}`, () => {

it(`should not raise func name error for event parameters ${key}`, () => {

dbale-altoros · 2023-08-01T15:29:10Z

test/rules/naming/modifier-name-mixedcase.js

+    }
+
+    for (const [key, code] of Object.entries(WITH_$)) {
+      it(`should not raise func name error for functions ${key}`, () => {


==> for modifiers
(wrong description on IT statement)

Suggested change

it(`should not raise func name error for functions ${key}`, () => {

it(`should not raise func name error for modifiers ${key}`, () => {

dbale-altoros · 2023-08-01T15:44:31Z

I will close this PR
I opened this one
#465

With these modifications and fixes

ernestognw · 2023-08-01T15:56:58Z

I will close this PR I opened this one #465

With these modifications and fixes

Yeah, I got also a bunch of copy paste errors, sorry for that. No issue having another PR, thanks for the review!

Allow use of variables starting with $ in var-name-mixedcase

3a10a5e

ernestognw mentioned this pull request Jul 14, 2023

Allow Initializable versions greater than 256 OpenZeppelin/openzeppelin-contracts#4460

Merged

3 tasks

dbale-altoros added the awaiting user feedback awaiting user feedback label Jul 17, 2023

dbale-altoros changed the base branch from master to develop July 19, 2023 13:27

ernestognw marked this pull request as draft July 19, 2023 17:01

Complete $ support for every identifier naming

3922c78

ernestognw marked this pull request as ready for review July 19, 2023 20:44

ernestognw changed the title ~~Allow use of variables starting with $ in var-name-mixedcase~~ Allow use of variables starting with $ in mixedcase rules Jul 25, 2023

ernestognw changed the title ~~Allow use of variables starting with $ in mixedcase rules~~ Allow $ in casing-related rules Jul 25, 2023

dbale-altoros added awaiting for review and removed awaiting user feedback awaiting user feedback labels Jul 26, 2023

Merge branch 'develop' into chore/support-$-in-var-name-mixedcase

d1da074

dbale-altoros reviewed Aug 1, 2023

View reviewed changes

dbale-altoros closed this Aug 1, 2023

dbale-altoros removed the awaiting for review label Aug 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow `$` in casing-related rules #453

Allow `$` in casing-related rules #453

ernestognw commented Jul 14, 2023 •

edited

Loading

dbale-altoros commented Jul 17, 2023

ernestognw commented Jul 17, 2023

dbale-altoros commented Jul 18, 2023

ernestognw commented Jul 18, 2023

dbale-altoros commented Jul 19, 2023

dbale-altoros commented Jul 19, 2023

ernestognw commented Jul 19, 2023

dbale-altoros commented Jul 20, 2023

dbale-altoros commented Aug 1, 2023

dbale-altoros Aug 1, 2023

ernestognw Aug 1, 2023

dbale-altoros Aug 1, 2023

ernestognw Aug 1, 2023

dbale-altoros Aug 1, 2023 •

edited

Loading

ernestognw Aug 1, 2023

dbale-altoros Aug 1, 2023

ernestognw Aug 1, 2023

dbale-altoros commented Aug 1, 2023

ernestognw commented Aug 1, 2023

	it(`should not raise event name error for events ${key}`, () => {
	it(`should not raise event name error for constants ${key}`, () => {

	rules: { 'contract-name-camelcase': 'error' },
	rules: { 'event-name-camelcase': 'error' },

	it(`should not raise func name error for functions ${key}`, () => {
	it(`should not raise func name error for event parameters ${key}`, () => {

Allow $ in casing-related rules #453

Allow $ in casing-related rules #453

Conversation

ernestognw commented Jul 14, 2023 • edited Loading

dbale-altoros commented Jul 17, 2023

ernestognw commented Jul 17, 2023

dbale-altoros commented Jul 18, 2023

ernestognw commented Jul 18, 2023

dbale-altoros commented Jul 19, 2023

dbale-altoros commented Jul 19, 2023

ernestognw commented Jul 19, 2023

dbale-altoros commented Jul 20, 2023

dbale-altoros commented Aug 1, 2023

dbale-altoros Aug 1, 2023

Choose a reason for hiding this comment

ernestognw Aug 1, 2023

Choose a reason for hiding this comment

dbale-altoros Aug 1, 2023

Choose a reason for hiding this comment

ernestognw Aug 1, 2023

Choose a reason for hiding this comment

dbale-altoros Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

ernestognw Aug 1, 2023

Choose a reason for hiding this comment

dbale-altoros Aug 1, 2023

Choose a reason for hiding this comment

ernestognw Aug 1, 2023

Choose a reason for hiding this comment

dbale-altoros commented Aug 1, 2023

ernestognw commented Aug 1, 2023

Allow `$` in casing-related rules #453

Allow `$` in casing-related rules #453

ernestognw commented Jul 14, 2023 •

edited

Loading

dbale-altoros Aug 1, 2023 •

edited

Loading