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

feat: support globs in extends #853

Merged
merged 3 commits into from
Oct 22, 2024

Conversation

mrexox
Copy link
Member

@mrexox mrexox commented Oct 21, 2024

Closes #852

⚡ Summary

Support globs in extends option.

  • Expand the possible globs before extending the main config
  • Add tests

Probably need to change checksum check from the file checksum to a memory object checksum – #854

@mrexox mrexox marked this pull request as ready for review October 21, 2024 08:09
@mrexox mrexox merged commit f3b1a81 into evilmartians:master Oct 22, 2024
15 of 19 checks passed
@VanTanev
Copy link

VanTanev commented Oct 23, 2024

Hey, for some reason I didn't get a notification about your response in the discussion, sorry for getting back to you late!

This is a good step forward, but I think it needs one more improvement to be viable.

Configs that are extended should automatically set the commands.{}.root parameter to the folder of the extended config file.

Example, given:

# ./lefthook.yml
extends:
 - "apps/*/lefthook.yml"

and

# ./apps/store/lefthook.yml
pre-commit:
  commands:
    jest:
      run: pnpm jest --findRelatedTests --passWithNoTests {staged_files}

It will fail with:

ERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL  Command "jest" not found

To fix it, one needs to manually add root: apps/store

# ./apps/store/lefthook.yml
pre-commit:
  commands:
    jest:
      root: apps/store # <!------ This needs to be added to every single command, even though it could be inferred from the file location.
      run: pnpm jest --findRelatedTests --passWithNoTests {staged_files}

Which is very clunky.

@mrexox
Copy link
Member Author

mrexox commented Oct 23, 2024

Oh, I see. Yes, it might be too much to add root to every command, but now this is a must. I don't think it's a good idea to automatically add root because you can merge lots of files from different folders, and this can become too complicated to understand if something is added automatically.

Currently I don't have an elegant solution yet, so I have nothing to suggest except for explicit configuration for every command.

@VanTanev
Copy link

Yeah, the correct solution is not obvious. For example, lint-staged has the reverse approach, where leaf config files import from a root file, so it makes sense that commands are relative to leaf config file locations.

I do think that in the general case, automatically setting root for leafs should be fine, but there are definitely edge-cases, and such a change would be semver-breaking.

So, ultimately we're stuck with some repetition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants