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

Added property validUndefinedVariableRegexp to VariableAnalysisSniff #173

Merged
merged 2 commits into from
Jun 11, 2020

Conversation

khromalabs
Copy link
Contributor

@khromalabs khromalabs commented May 13, 2020

Hi this property is useful for me as in my company they follow a name standard for declaring global variables (I don't like that, but it's annoying to constantly pollute the undeclared warnings with those). Just in case you'd like to include this in the master branch. Thanks for this great project BTW.

@jrfnl
Copy link
Collaborator

jrfnl commented May 13, 2020

@khromalabs Not a bad idea ;-)

I'd like to suggest for this property to be an array.

For projects without a naming convention for global variables, it might be easier to just provide a list of variable names.
Along the same lines, a regex could become really complex if it would need to handle a set conventions, so being able to provide several regexes might make that more manageable.

Reading the above back, I'm even wondering if there should be two properties: one to provide a list with the names of variables, one to provide a list of regexes.

@sirbrillig
Copy link
Owner

Yeah, this looks great! More configurability is very useful. Thank you. I'd be fine with the option as-is, but I have been wanting to use arrays for several of the other options so maybe this is a good time to make one of them that way as @jrfnl suggests. If that's too much work for you right now we could always get this in now and add another option that includes arrays later. I'll leave that to your judgement.

At the very least, we'll need some tests to cover this. You should be able to duplicate and modify this test to cover your use case. Let me know if you need any help doing so!

@khromalabs
Copy link
Contributor Author

khromalabs commented May 25, 2020

[..]

but I have been wanting to use arrays for several of the other options so maybe this is a good time to make one of them that way as @jrfnl suggests.

[..]

At the very least, we'll need some tests to cover this. You should be able to duplicate and modify

Sorry about my radio silence, I wanted to take a look at this last weekend but I ended up reinstalling whole OS in my laptop... so I'll try this week.

@sirbrillig
Copy link
Owner

sirbrillig commented Jun 11, 2020

Looks good! Thank you!

I'll add it to the README in a separate PR.

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