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

Variable Formatting: global constants #42

Open
EdJoPaTo opened this issue Oct 4, 2021 · 11 comments
Open

Variable Formatting: global constants #42

EdJoPaTo opened this issue Oct 4, 2021 · 11 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@EdJoPaTo
Copy link

EdJoPaTo commented Oct 4, 2021

Related to the rule @typescript-eslint/naming-convention.

I regularly use global constants with UPPER_CASE style for constants that wont change in runtime. Not sure how to exactly specify that in the rules but I first wanted to suggest something like that here before checking on how that could be done.

Something like these:

const DIRECTORY = './events'
const DEFAULT_OPTIONS: Options = {}

If thats something that is not liked here: How would you propose to write constants like this instead?

@sindresorhus
Copy link
Member

It's intentional: eslint/eslint#10605

@sindresorhus
Copy link
Member

sindresorhus commented Oct 4, 2021

@fregante What are your thoughts about this?

@fregante
Copy link
Member

fregante commented Oct 4, 2021

Perhaps they should be allowed only at the top level. They're pretty common, even though I don't particularly see the need for that.

@EdJoPaTo
Copy link
Author

EdJoPaTo commented Oct 4, 2021

I'm not sure if they are useful in libraries as they objective is to be configurable. But I have a bunch of them in projects that running somewhere doing something.

As a "need" I searched for some examples from my code (rg "^(?:export )?const [A-Z][A-Z_]+" --glob "*.ts"):

  • SOMETHING_REGEX to be used in new RegExp()
  • DEFAULT_… used as default values somewhere
  • EXAMPLE_… used by multiple tests
  • BASE_… used in objects where some are always the same ({...BASE_BLA, …})
  • numeric constants to simplify calculations (const SECONDS_PER_DAY = 60 * 60 * 24) or being used as sleep / interval arguments later on
  • folder or files where stuff is written or read
  • emojis (and other non ascii stuff) which are simpler to use by writing ERROR_EMOJI rather than the exact emoji / unicode thingy.

in general: stuff that wont change at runtime / compile time constants

I can use camelcase here but I am kinda used to seeing constants directly. Also they are conventions on other c derived languages (example). I am relatively sure I am not the only xo user using them. Also my other currently used language (Rust) produces a warning in that case: non-upper-case-globals but in contrast to TypeScript it knows when something is really const or not.

@sindresorhus
Copy link
Member

Yeah, I guess upper-case is a good way to make it clear that the value is completely static. PR welcome.

@sindresorhus
Copy link
Member

Perhaps they should be allowed only at the top level. They're pretty common, even though I don't particularly see the need for that.

Ideally, yes, but there's no way to enforce that with this rule.

@sindresorhus sindresorhus added enhancement New feature or request help wanted Extra attention is needed labels Oct 5, 2021
@EdJoPaTo
Copy link
Author

EdJoPaTo commented Oct 5, 2021

There is. The first part of the rule description even states that:

such as by enforcing all private properties begin with an _, and all global-level constants are written in UPPER_CASE.

Selectors → modifiers → global

@sindresorhus
Copy link
Member

Oh great. I missed that.

@EdJoPaTo
Copy link
Author

EdJoPaTo commented Oct 5, 2021

I tested around with the following but it didnt seem to work… Not sure why…

{
	selector: 'variable',
	format: [
		'strictCamelCase',
		'UPPER_CASE'
	],
	modifiers: [
		'global',
		'const'
	]
},

@kestrelnova
Copy link

It's because of how the rule prioritizes the selectors. If you have two selectors for variable, one with the filter option and one without, a variable name will always be checked against the one with the filter first.

So because of that, the very first selector in the XO rule ends up overriding any other variable selectors added later (unless they also have filters). Because it has a filter, a variable name will always be checked against it first, and the name will always match because the only criteria is that it doesn't have a hyphen or space (which variables can't have anyway).

There are a few ways to get around this, but the simplest way is just to remove the filter entirely. Properties with hyphens or spaces will still be exempt from camelCase because of the later requiresQuotes selector; I'm not sure if the filter was intended to do anything else?

Once that's removed, the above code for global constants will mostly work as intended. The only issue is that it does conflict with another selector for booleans. Since that one specifies types, it has a higher priority than selectors that only specify modifiers, so any booleans will get matched there first, global constants or not.

(I don't think that boolean rule actually triggers at all right now, because of the same issue with the filter. I didn't get any errors from it until I removed the filter, and then I suddenly got a whole bunch.)

So if you want global constant booleans to have the required prefixes and potentially be UPPER_CASE, you need to add a special case for that, something like this:

{
  selector: 'variable',
  types: ['boolean'],
  modifiers: ['const', 'global'],
  format: ['UPPER_CASE'],
  prefix: ['IS_', 'HAS_', 'CAN_', 'SHOULD_', 'WILL_', 'DID_'],
  filter: '[A-Z]{2,}'
},

Then adding everything together should get the right output.

@maxpain
Copy link

maxpain commented Jan 13, 2022

Any updates on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants