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

Add linter for duplicate features #4200

Closed
connorshea opened this issue May 22, 2019 · 8 comments
Closed

Add linter for duplicate features #4200

connorshea opened this issue May 22, 2019 · 8 comments
Labels
enhancement 🥇 Nice to have features. linter 🏡 Issues or pull requests regarding the tests / linter of the JSON files.

Comments

@connorshea
Copy link
Contributor

connorshea commented May 22, 2019

For example, if someone copy-pastes an existing JSON file when tracking a new feature (which I definitely have done a lot) and then forgets to change the feature title, you could have the same feature path (e.g. css.properties.-webkit-box-reflect) for two separate features.

// In one file.
{
  "css": {
    "properties": {
      "-webkit-box-reflect": {
          // etc.
      }
    }
  }
}

// In another file.
{
  "css": {
    "properties": {
      "-webkit-box-reflect": {
          // etc.
      }
    }
  }
}

I don't think this would ever cause false-positives, since as far as I know you're never supposed to have duplicate feature names anyway.

I'm not sure how exactly to write a performant means of doing this, but I imagine someone smarter than me could figure it out :)

This would help detect issues like the one fixed in #4199.

@connorshea
Copy link
Contributor Author

I'm pretty sure I suggested a fix for this at some point before, but it was about requiring that the filename and main feature name match, and that was too likely to have false positives. Hopefully this one would work better? :)

@queengooborg queengooborg added enhancement 🥇 Nice to have features. linter 🏡 Issues or pull requests regarding the tests / linter of the JSON files. labels May 22, 2019
@ddbeck
Copy link
Collaborator

ddbeck commented May 22, 2019

I think part of the thinking behind having the full hierarchy in every file was that we might want to split up very large APIs into multiple files (for instance, splitting subfeatures of window across multiple files). I don't think we actually do this anywhere, at least not yet, but it might be good to to make sure and also ask ourselves whether we ever intend to actually intend to split up large features before taking away the option.

@connorshea
Copy link
Contributor Author

connorshea commented May 22, 2019

@ddbeck it shouldn’t effect that use case since we’d only want to detect duplication at the tips of the tree, so you could separate subfeatures of a feature into multiple files, and it wouldn’t trigger the linter unless you repeated the parent feature’s support info or had the same subfeature in multiple files.

@ddbeck
Copy link
Collaborator

ddbeck commented May 22, 2019

🤦‍♂ OK, yeah, that makes sense! I misunderstood this as detecting duplication of a css.properties.-webkit-box-reflect in general, not css.properties.-webkit-box-reflect.__compat specifically.

@connorshea
Copy link
Contributor Author

I probably could have been more clear on that point :)

@Elchi3
Copy link
Member

Elchi3 commented Feb 9, 2021

This becomes a higher priority with the new mixin approach: #8929

@queengooborg
Copy link
Collaborator

queengooborg commented Mar 27, 2022

Duplicate features across multiple files has been basically disallowed by #9821, and a better error message for it in #15479. Duplicate features in the same file are still "allowed", but will be resolved by npm run fix in an undefined manner. (I'm not sure if we can lint for duplicate features in the same file...)

@queengooborg
Copy link
Collaborator

With the better error message in #15479, I think that this has been resolved!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🥇 Nice to have features. linter 🏡 Issues or pull requests regarding the tests / linter of the JSON files.
Projects
None yet
Development

No branches or pull requests

4 participants