[Feature request] Improved diff options, ability to specify branch

[JeremyMcConnell]

So glad someone made this into a plugin, I'm tired using hacky scripts to get this done.

It would be great if we could have more flexibility with how the diff is generated. Instead of comparing to the HEAD, comparing to another branch adds a lot of capability. Specifically for comparing against authoritative branches. For example, git diff master... or git diff production.... It would make this usable in CI tools and PR checks. That way team doesn't have to learn a new habit either.

I'd be willing to contribute something if you're open to this idea.

[paleite]

Hi Jeremy,

I think this sounds like a fantastic idea and I wish I had thought of it myself 😃

We could for example add this functionality by setting the argument for what to diff in eslintrc’s settings-property (like eslint-plugin-react does).

Eg settings: { diff: { arguments: [“production...”] } }

I would be thrilled if you could contribute, because I would love to implement it myself, but can’t find the time, currently.

Thanks for trying out the plugin and thanks for taking the time to provide feedback.

Cheers 🙂

On Wed, 7 Oct 2020 at 20:50 Jeremy McConnell notifications@github.com wrote:

So glad someone made this into a plugin, I'm tired using hacky scripts to get this done.

It would be great if we could have more flexibility with how the diff is generated. Specifically for comparing against authoritative branches. For example, git diff master... or git diff production.... It would make this usable in CI tools and PR checks. That way team doesn't have to learn a new habit either.

I'd be willing to contribute something if you're open to this idea.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/paleite/eslint-plugin-diff/issues/1, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBZVQEUVI4NB5L6ADE5P33SJS2BDANCNFSM4SHYX7NQ .

[JeremyMcConnell]

@paleite Sounds great, I'll get something started over the weekend

[bytenik]

I'd be happy to help with this as well @JeremyMcConnell; I was thinking about writing something similar and am glad I found out you're doing the same. I'd likely want to lint only changes introduced in commits involved in a build on my CI environment.

[paleite]

That's awesome 🙂

Do you guys reckon the CI should be able to set the diffing-point via an environment variable then? E.g. ESLINT_PLUGIN_DIFF_BRANCH=develop Or would you hard-code it as a setting or do you propose some other solution?

Can you pass variables to ESLint plugins?

[bytenik]

Hmm, I suppose I don't need to select a branch as much as I need to select a ref. My CI will tell me the last built commit and I can then pass that in via env var, so that the diff is done between that commit and the head. The branch would remain the same.

An env var would make that much easier.

What I would actually love would be to set the commit range applicable for each rule, but, I don't think eslint will give you that level of flexibility.

[JeremyMcConnell]

@paleite @bytenik

I finally got around to looking into some options here.

Eslint has an established config property for plugin settings which I'm seeing other plugins use, see https://eslint.org/docs/2.0.0/user-guide/configuring#adding-shared-settings. I figure this is better than an env because the config can just grab any arbitrary env. This gives a lot of flexibility but also keeps in the eslint unofficial convention.

I went down a rabbit hole here and also removed the diff/staged configs to avoid having two ways of configuring the plugin.

So new configs would look something like this:

module.exports = {
  parserOptions: {},
  env: {},
  plugins: ['diff'],
  overrides: [
    {
      files: ['*.*'],
      processor: 'diff/diff'
    }
  ],
  settings: {
    'diff/git-diffing-params' : () => {

           // My use case
           return 'authoritative-release... --unified=0'

           // existing 'staged' use case 
           return '--diff-filter=ACM --unified=0 HEAD -- staged'

           // bytenik's use case
          return  'head...' + process.env.CI_BUILD_REF' + ' --unified=0'
      }
  }
};

What do you guys think? I've got this mostly implemented but can't find the time to clean it up and test (hopefully next week).

[bytenik]

That looks great. Can you push it to a public repo somewhere? Maybe I can finish it up. My wife told her work about this and now they're really excited to use it too. 😁 I think there's a ton of applications for this plugin.

[paleite]

Sounds good! Go ahead and create a PR, then we can pick it up from there.

And thanks for all your efforts so far!

[bytenik]

@JeremyMcConnell Were you able to get anywhere with this? If you can open a PR or push it to a fork or something, I can pick it up from here.

[paleite]

Please update to 1.0.6 and see if that helps.

To do a diff like git diff master..., you can specify the range like so: ESLINT_PLUGIN_DIFF_COMMIT="master..." yarn run eslint .

Please let me know if you run into any issues.

More info: https://github.com/paleite/eslint-plugin-diff/releases/tag/v1.0.6

[paleite]

The new CI support should address the feature request mentioned here, as well: https://github.com/grvcoelho/lint-diff/issues/6#issuecomment-668887489