feat(fs-access-mode-constants): introduce

[nekojanai]

closes #130

[JakobJingleheimer]

Nice! Already onto the second one 😀 Thanks for this!

#NamingIsHard I think this should be called fs-access-mode-constants (it affects more than only files).

[AugustinMauroy]

Not too bad. test file should be something like file-x.js

[JakobJingleheimer]

Is there a way to detect this leveraging ast-grep? (I think that would be more robust)

If not, I think you probably want a regex instead of hard strings. And also, I think you don't need to first test whether there is a match before trying to replace that match: you can feed string.replace a regex as the first arg; when there is no match, it will just do nothing (aka "fail gracefully").

Suggested change

	if (originalText.includes(", }") || originalText.includes(", ") || originalText.includes(" , ")) {
	const newText = originalText.replace(" , ", "").replace(",,", ",").replace(", ", ", ").replace(", }", " }");
	const newText = originalText
	.replace(/\s+,+\s+/, ' , ')
	.replace(/\{\s*,/, '{ ')
	.replace(/,\s*\}/, ' }');
	if (newText !== originalText) {

You should test those regexes though (I think they're right, but I didn't confirm)

[AugustinMauroy]

@brunocroh do we have a utility for that ?

[brunocroh]

Yes, we have an utility function utils/src/ast-grep/remove-binding.ts that can be used here

  let hasChange = false
  for (const key of ['F_OK', 'R_OK', 'W_OK', 'X_OK']) {
    const change = removeBinding(importStatement, key); //it will return a ast-grep.Edit to remove import if it exists
    if(change) {
       edits.push(change.edit)
       hasChange = true
    }
  }
  
  if(hasChange) {
    // add constants to import
  }

[brunocroh]

An easier way to handle destructured objects or named imports is

// get items inside of objectPattern
const imports = objectPattern.find({
  rule: {
    any: [
     {kind: 'import_specifier'},
     {kind: 'shorthand_property_identifier_pattern'}
    ]
  }
})
//now imported items will be in `imports` variable as a string array, that will be easier to transform, i.g
// imports.push('newImport')
// items.filter // remove import
// items.map // update import

// after transform just join the array with ", "
const newImportString = `{ ${newImports.join(", ")} }`

// Replace the entire object pattern with new object pattern
objectPattern.replace(newImportString)

[nekojanai]

An easier way to handle destructured objects or named imports is

// get items inside of objectPattern
const imports = objectPattern.find({
  rule: {
    any: [
     {kind: 'import_specifier'},
     {kind: 'shorthand_property_identifier_pattern'}
    ]
  }
})
//now imported items will be in `imports` variable as a string array, that will be easier to transform, i.g
// imports.push('newImport')
// items.filter // remove import
// items.map // update import

// after transform just join the array with ", "
const newImportString = `{ ${newImports.join(", ")} }`

// Replace the entire object pattern with new object pattern
objectPattern.replace(newImportString)

I tried getting the import specifiers in an array via the ruleset provided above but going off of

    const objectPattern = statement.find({
      rule:
        { kind: 'object_pattern' },
    });

that didn't yield any results so I took a different approach

[nekojanai]

incorporated feedback and adjusted coded accordingly
@brunocroh I choose not to use the utility function mentioned

my current approach allows me to handle every require and import statements with arrays containing the specifiers, and conditional replacements instead of removal is more streamlined imo

[brunocroh]

incorporated feedback and adjusted coded accordingly @brunocroh I choose not to use the utility function mentioned

my current approach allows me to handle every require and import statements with arrays containing the specifiers, and conditional replacements instead of removal is more streamlined imo

That's fine, don't worry!

[AugustinMauroy]

LGMT with @brunocroh suggestion

[JakobJingleheimer]

I think this is very close!

P.S. Sorry for my lag (been very busy).

[JakobJingleheimer]

@nekojanai please stop force-pushing 🙏 Doing that makes it very difficult to review changes over time; instead of being able to see the subset of new changes, the reviewer has look at everything again because the periodic diff is broken.

[JakobJingleheimer]

w00t! I think this is good to go (nits can be addressed in a follow-up if appropriate).

[JakobJingleheimer]

Is this deliberately using a binary OR (|) instead of a logical OR (||)?

[JakobJingleheimer]

@nekojanai

[JakobJingleheimer]

Is condition supposed to be mode? (or vice versa)

[JakobJingleheimer]

@nekojanai could you answer about this though? (I know it doesn't especially matter, so fine to merge without the answer)

[JakobJingleheimer]

Nit: the formatting makes this a bit difficult to grok

Suggested change

	let objPatArr = objectPattern.findAll({
	rule:
	{ kind: 'import_specifier' },
	}).map((v) => v.text());
	let objPatArr = objectPattern.findAll({
	rule: { kind: 'import_specifier' },
	}).map((v) => v.text());

[JakobJingleheimer]

Nit: this array is declared and used multiple times. It could instead be declared as a const in the root scope and then referenced.

[brunocroh]

LGTM, Awesome work, thank you!

[nekojanai]

uhhh, LGTM? @brunocroh @JakobJingleheimer @AugustinMauroy

[JakobJingleheimer]

Yes, it's approved. I thought you might want to press the green button, but I can do.