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

Wildcards in whitelist #4504

Open
SmileyFtW opened this issue Nov 7, 2018 · 11 comments
Open

Wildcards in whitelist #4504

SmileyFtW opened this issue Nov 7, 2018 · 11 comments
Labels
enhancement Feature requests, or enhancements to existing features. Ideas. Anything within the project's scope. feature-settings status-ignored The team ignores this issue. Not something we want to implement ourselves, but would accept. up-for-grabs Use this label in conjunction with a difficulty level label, e.g. difficulty-02-ducky

Comments

@SmileyFtW
Copy link

It would be nice to be able to use wildcards for names in the code inspection whitelist:
txtPlayerNumber1, txtPlayerNumber2, txtPlayerNumber3, etc, could be specified in a single entry as txtPlayerNumber#

@retailcoder
Copy link
Member

The whitelist doubles as a list of valid prefixes and identifiers: txt would remove all "Hungarian notation" results for txt* identifiers.

Use meaningful names would probably still fire results for numbered identifiers though... but it's specifically designed to do so. Numbered identifiers are a code smell; in a vast majority of cases, it is preferable to use an array or a collection of some sort - hence the by-design nature of flagging numbered identifiers as poor names. In VB6 you could have control arrays, unfortunately that isn't an option with MSForms in VBA.

I don't completely disagree with supporting wildcards, but it seems to enable mass-undermining the use meaningful names inspection... something that can also be done by simply disabling it.

If the number of players is unknown at compile-time, consider creating the textbox controls at run-time - in which case it becomes easy to use a name that doesn't trigger the inspection, and then you can craft a UI that is more representative of your domain model. OTOH if there's a fixed number of players and the UI can reasonably be made at design-time, I'd probably still want to whitelist each control name individually.

So... I'm kinda torn on this one.

@retailcoder retailcoder added enhancement Feature requests, or enhancements to existing features. Ideas. Anything within the project's scope. feature-settings labels Nov 8, 2018
@daFreeMan
Copy link
Contributor

Player1, Player2 ... PlayerX are pretty special cases for numbered identifiers where most sane people would say "yeah, those make sense". As such, I'm in the "Whitelist them individually" camp because there are cases where there are sane, logical reasons for giving identifiers such names. Besides, how many players are there going to be in your game?

If you're going to allow a wildcard, do you have to support . to represent a single character and * to represent 1 or more (or is that 0 or more) characters? Do you go full RegEx. Do you use file system globbing rules?

@retailcoder
Copy link
Member

Player1, Player2 ... PlayerX are pretty special cases for numbered identifiers where most sane people would say "yeah, those make sense"

Depends. If I'm writing a game of Battleship, yes (fixed players, few players: Player1, Player2). If I'm making a game of Monopoly or Risk (variable players, more than 2 max players), or a game where the number of players is unknown and the maximum is arbitrary or irrelevant, then I'd rather have a players collection and avoid the numbering altogether.

As for wildcards, I'd go with ? for 0-1 character, and * for 0-n characters (regex 0-n is *, 1-n is +)... i.e. DOS-style. Regex and pattern matching seems overkill.

@retailcoder
Copy link
Member

Regex and pattern matching seems overkill.

OTOH, this is an IDE, and we're matching identifiers. Pattern matching does make sense... But then I'd go RegEx over Like syntax, but then regular expressions aren't exactly trivial for everyone.. so maybe a flag to enable regex patterns, otherwise default to filesystem wildcards... and now this feels like scope/feature creep... for a feature that ultimately will use this to allow bad variable naming to go unflagged: a pattern like * essentially makes the entire inspection work hard for nothing.

The crux of the problem seems to be that it's impossible to annotate a form control with an @Ignore annotation. Not sure the solution is to make the whitelist support wildcards.

I'll keep this issue open and up-for-grabs, and won't refuse a PR that implements it... But I'm not sold at all on it being a good idea.

@Vogel612
Copy link
Member

Vogel612 commented Nov 8, 2018

An idea for implementation might be something like the following:

Allow Wildcard specifications with *, # and & for "anything", "number" and "text" These wildcards are not regex wildcards, but could be transformed to "proper regex" as follows:

* -> .*
# -> \d+
& -> \w+

patterns that only contain wildcards could be rejected.
For better speed, the compiled regex patterns should be compiled and cached. Invalidating that regex cache is trivially done when the list of ignored identifiers is changed.

Regex matches should be bounded on both ends with an anchor.

@Vogel612 Vogel612 added up-for-grabs Use this label in conjunction with a difficulty level label, e.g. difficulty-02-ducky status-ignored The team ignores this issue. Not something we want to implement ourselves, but would accept. labels Nov 8, 2018
@bclothier
Copy link
Contributor

I also wonder if this would be better if instead of wildcard, we only allow a range, which would close the range that could be match:

Player[1-4] => permits only 4 entries
Command([En]|[Dis])abled => permits only 2 entires; CommandEnabled or CommandDisabled.

This way, those can be expanded into an array behind the scene. That wouldn't then require regex matching at all.

@comintern
Copy link
Contributor

@retailcoder - If the root issue is not being able to annotate @Ignore a form control, there are a couple ways we could address that specifically without descending down the rabbit hole of custom patterns at all.

One would be to allow whitelisting of AsTypeNames, for example allow whitelisting MSForms.Control or Excel.TextBox as types.

Another would be to extend the @Ignore annotation to allow for an identifier or class at the module level. Something like @Ignore HungarianNotation(MSForms.Control).

@retailcoder
Copy link
Member

One would be to allow whitelisting of AsTypeNames, for example allow whitelisting MSForms.Control or Excel.TextBox as types.

Effectively whitelisting TextBox1 and ComboBox12?

@comintern
Copy link
Contributor

That was my thought. Hell, you could use it to ignore oFoo and still get results for strFoo. By whitelisting Object.

@SmileyFtW
Copy link
Author

SmileyFtW commented Nov 8, 2018 via email

@SmileyFtW
Copy link
Author

SmileyFtW commented Nov 8, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests, or enhancements to existing features. Ideas. Anything within the project's scope. feature-settings status-ignored The team ignores this issue. Not something we want to implement ourselves, but would accept. up-for-grabs Use this label in conjunction with a difficulty level label, e.g. difficulty-02-ducky
Projects
None yet
Development

No branches or pull requests

6 participants