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

`OR` relationship with `types` #607

Closed
asottile opened this issue Sep 5, 2017 · 20 comments
Closed

`OR` relationship with `types` #607

asottile opened this issue Sep 5, 2017 · 20 comments
Labels

Comments

@asottile
Copy link
Member

@asottile asottile commented Sep 5, 2017

When we introduced this feature, I had some specific usecases in mind which required an AND relationship with types -- it comes to mind that some tools would like to use an OR relationship, but there isn't a great way to implement this without reverting to files (and thus losing the benefits of types altogether!).

This issue is to track some sort of way of enabling an OR relationship.

This was originally inspired by prettier/prettier#2745

@asottile
Copy link
Member Author

@asottile asottile commented Mar 10, 2018

I don't want to invent a meta-language for this -- it'll be confusing, complicated, and doesn't sound like something I want to maintain.

Here's an idea I've been thinking of:

Add a types_or entry, this will be anded with the current types and files entries, but internally will be ored.

This wouldn't enable all potential combinations that you could think up (stuff like (js and file) or (python and symlink) wouldn't be supported) but I think it solves the realistic basic case (file and (js or css or ...))

@zph
Copy link

@zph zph commented May 15, 2018

@asottile Thinking of #744 and based on that implementation of what I thought the desired behavior was... I think having a separate key for types_or is the rough concept I'd go for. With a hard requirement of only allowing either a types or a types_or declaration. Merging both seems messy and undesirable.

Unless we took the following approach:

does it match all of types && at least one of types_or

That could work too. I'll mull on it. If I have the time and inclination I'll put up a proposal and a viable solution :).

@SimonHeimberg
Copy link

@SimonHeimberg SimonHeimberg commented Nov 16, 2018

alternative suggestion for config:

types:
    - [ type1A, type1B, type1C]
    - [ type2A, type2B ]

in programming terms: make types optional a two-dimensional array

type must match (type1A and type1B and type1C) or (type2A and type2B)

@asottile
Copy link
Member Author

@asottile asottile commented Nov 16, 2018

That adds a level of complexity I don't really want:

  • it's an incompatible schema to what we have today
  • it's not obvious at a glance what it means
@SimonHeimberg
Copy link

@SimonHeimberg SimonHeimberg commented Nov 17, 2018

Why would it be incompatible?

types: [file, java] # to use for usual cases, would still work
types:
  - [file, java]
  - [file, css]
  - [symlink, python]
# above only used for special cases

Well, the current files/types/... is already not obvious for everybody, as #706 shows

@asottile
Copy link
Member Author

@asottile asottile commented Nov 17, 2018

Why would it be incompatible?

Existing versions of pre-commit would error on list-of-lists

Well, the current files/types/... is already not obvious for everybody, as #706 shows

They are at least documented and consistent (everything today is AND). Though #706 was a pretty spectacular series of incorrect assumptions and lack of reading the docs ;)

@gtback
Copy link

@gtback gtback commented Sep 25, 2019

I ran into this same issue today with the eslint hook and went down the "why is this not working as an OR?" rabbit hole before busting out ipdb, reading the source, and finding this issue. eslint is currently adding support for Typescript files in addition to JavaScript. I ended up using a similar hack:

types: [file]
files: \.(js|ts)$

(I actually had just the second line in my file for a while, but it wasn't working, before I dug in more and learned about types).

I'm happy to keep this locally in my repo, but had some other ideas:

  • Modify the eslint hook definition to not use types. By default, eslint only looks at .js files, so my hook also includes:
    args:
    - --ext
    - .js,.ts
    I can make an issue in the mirrors-eslint repo if this is the best solution.
  • Update identify to have a "meta-type" for .js(x?) and .ts(x?) files. I'm not sure what it would be called, and I wonder if that's stretching the purpose of identify too far. But it it's appropriate, I can open an issue there.
  • Wait for an types_or (or similar) to be added here. I'd be happy to work on a PR if the consensus is that this is the way to go.
@Buuntu
Copy link

@Buuntu Buuntu commented Mar 31, 2020

types: [file]
files: \.(js|ts)$

Did you ever find out how to solve this? I can't get my eslint hook to pick up .ts or .tsx files.

@gtback
Copy link

@gtback gtback commented Apr 3, 2020

@Buuntu did you also add the eslint args? By default eslint only picks up .js files, I believe

args:
- --ext
- .js,.ts,.tsx
@magicmark
Copy link
Contributor

@magicmark magicmark commented Jul 23, 2020

just a data point for this being useful - this bit me today, spent a few cycles tryna work out what was going wrong (thanks @chriskuehl for pointing me right!)

@MarcoGorelli
Copy link
Contributor

@MarcoGorelli MarcoGorelli commented Oct 9, 2020

Add a types_or entry, this will be anded with the current types and files entries, but internally will be ored.

Hi @asottile - would you still be open to supporting this? I could try submitting a PR if so

EDIT

Haven't got round to this yet, but it's still on my to-do list

@asottile
Copy link
Member Author

@asottile asottile commented Oct 9, 2020

yeah nobody has stepped up with a better proposal or opposition

@henryiii
Copy link

@henryiii henryiii commented Oct 13, 2020

Note, https://github.com/actions/labeler does this by allowing any: [] and all: [] or a list. Might be useful for inspiration?

Edit: Looks like syntax is decided? Too bad, I feel it's not ideal. I think if, for example, the black hook changed to:

- id: black
  types_or: [python, pyi]

Then a user adding types: [file] would be very surprised when this doesn't work (since it's still ANDed with the types_or). Also, this doesn't work on older versions of pre-commit anyway, so you would have to wait to add the hook until the new version was the minimum (since it's ANDed with types, so you'd have to drop types from the hook). It seems to me types is more important for hooks, as user code is free to use files:, but hooks should be using types. And there's good reason to have multiple types in hooks.

@asottile
Copy link
Member Author

@asottile asottile commented Oct 13, 2020

Then a user adding types: [file] would be very surprised when this doesn't work (since it's still ANDed with the types_or).

I think you're misunderstanding: types: [file] + types_or: [python, pyi] would still match both python and pyi (types: [file] is the default after all!)

Also, this doesn't work on older versions of pre-commit anyway, so you would have to wait to add the hook until the new version was the minimum

well obviously, you can't add new features and have them work in older versions? but fortunately there's minimum_pre_commit_version specifically created for this

@henryiii
Copy link

@henryiii henryiii commented Oct 14, 2020

new features and have them work in older versions

But I thought the argument against types taking something fancy was that it wouldn't work in old versions?

I think you're misunderstanding

I don't think so, if you have types_or: [python, pyi] in the upstream hook, and as a user you try to override it by setting types: [file] and files: .pyx$, it won't work, because it's still only allowing python and pyi. You'd now have to change your user code to also have or only have types_or: [files].

@asottile
Copy link
Member Author

@asottile asottile commented Oct 14, 2020

no the argument against that was because it's schema incompatible

and sure, the override of types_or is no different than it is today for types, I don't see you rpoint

@henryiii
Copy link

@henryiii henryiii commented Oct 14, 2020

To simply provide a custom file on an arbitrary hook, you currently need:

- id: any_hook
  types: [file]
  files: "..."

And after this, you will instead need:

- id: any_hook
  types: [file]
  types_or: [file]
  files: "..."

That is, unless you look up the definition of the hook and/or are sure it won't change, you have to override both now. This is not end-of-the-world, but doesn't seem ideal. I'm not really suggesting a better alternative, either, just pointing out an issue.

@henryiii
Copy link

@henryiii henryiii commented Oct 14, 2020

And changing from types: [python] to types_or: [python, pyi] for an upstream hook, like black, would cause downstream users of the hook to suddenly stop checking the files they think they are checking if they had something like:

- id: any_hook
  types: [file]
  files: "(.py|.pyi|.py.in)$"

(and suddenly stopping to test a set of previously checked file often is very hard to detect).

@asottile
Copy link
Member Author

@asottile asottile commented Oct 14, 2020

And after this, you will instead need: ...

no you wouldn't need types: [file], only types_or: [file]

And changing from types: [python] to types_or: [python, pyi] for an upstream hook, like black, would cause downstream users of the hook to suddenly stop checking the files they think they are checking

only if they weren't part of types_or, I doubt anyone is checking .py.in since it's not valid python source

@henryiii
Copy link

@henryiii henryiii commented Oct 14, 2020

So if black (for example), defined types_or in the "upstream" hook, but you defined types: [file] when you use it, then it would not be types_or: from black AND types from your usage?

I doubt anyone is checking

You can be doing anything in your own repo, you know what files you have. In the upstream hook, hopefully not! That was just an example, but one of the usages of types_or is to match unusual types that are not in the upstream hook. (the other common usage being filtering, which would still be okay here).

*.cmake.in/CMakeLists.txt.in, for example, is a completely valid file type, defined in CMake and explicitly supported in cmake-format, so I do exactly this for the cmake-format hook pretty much every where I use it. (though it's likely the exception for .in, not the norm)

jdufresne added a commit to jdufresne/black that referenced this issue Dec 19, 2020
Since pre-commit 2.9.0 (2020-11-21), the types_or key can be used to
match multiple disparate file types. For more upstream details, see:
pre-commit/pre-commit#607

Fixes psf#402
jdufresne added a commit to jdufresne/isort that referenced this issue Dec 19, 2020
Since pre-commit 2.9.0 (2020-11-21), the types_or key can be used to
match multiple disparate file types. For more upstream details, see:
pre-commit/pre-commit#607

Fixes PyCQA#402
jdufresne added a commit to jdufresne/black that referenced this issue Dec 20, 2020
Since pre-commit 2.9.0 (2020-11-21), the types_or key can be used to
match multiple disparate file types. For more upstream details, see:
pre-commit/pre-commit#607

Add the minimum_pre_commit_version to require pre-commit 2.9.0+.

Fixes psf#402
cooperlees pushed a commit to psf/black that referenced this issue Dec 31, 2020
Since pre-commit 2.9.0 (2020-11-21), the types_or key can be used to
match multiple disparate file types. For more upstream details, see:
pre-commit/pre-commit#607

Add the minimum_pre_commit_version to require pre-commit 2.9.0+.

Fixes #402
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

8 participants