-
Notifications
You must be signed in to change notification settings - Fork 414
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
Simpler structure for configuration. #715
Comments
Hello @Ferroin ! Thank you! |
|
some_label: |
1 similar comment
some_label: |
First of all thanks for creating and maintaining this awesome github action! The config I wanted to update:
|
I can't say whether it works perfectly or not, but I wrote an updated schema that's available on JSON Schema Store: https://json.schemastore.org/pull-request-labeler-5.json |
Same for me. I also see
|
I just upgraded to v5, saw that it was a breaking change and came here to see what the update is about. Especially this:
I mean yes it's super flexible, but I had to read this 5 times to understand what it's about and I have a hard time coming up with use-cases for it. I think I'll use 'any-glob-to-any-file' 99% of the time (cfr. old config). It's a bit sad and short-sighted that this complexity and verbose config file format is forced upon all users for some pretty niche use-cases. I definitely don't need this complexity and see it only as a nuisance. If there was a version of the action with v4 config file syntax but upgraded to node 20 I'd be using that for sure. I guess I'm off copy-pasting the following a few hundred times in all our labeler config files now 🙄 :
Update: Update 2: |
Description:
Instead of requiring every single level of keys below the top level in the configuration file to have a list as a value, restructure to use mappings whenever possible.
In effect, change the config structure so that the following:
instead is represented like this:
Justification:
The config format used in v4 utilized type based discrimination of the list items for each label in the config to maintain backwards compatibility despite the addition of the
all
andany
syntax. I would argue that requiring a list in this case was less than ideal (JS/TS should be able to easily differentiate between an array and an object, and that type of differentiation would have made the config a bit easier to understand logically), but the backwards compatibility aspect was useful because the new config parser still supported the old format.With v5 though, this habit of making value under each label be a list of objects or strings was kept and further propagated to the new ‘sub’ configuration items, despite the fact that the new config format is not at all backwards compatible with the old formats.
This has a number of distinct disadvantages for end users:
pull_request_target
hook instead of apull_request
hook means that an invalid config file won’t actually show any errors until after it’s merged, at which point it will fail workflow runs on unrelated PRs.I also strongly suspect that the code doing the parsing is, in fact, more complex than it would need to be if this proposal were implemented, so this may have long-term benefits for the maintainability of the code.
Are you willing to submit a PR?
Unfortunately not, as I have no background working with TypeScript and recognize that this will probably require some nontrivial changes to the code that parses the config file.
The text was updated successfully, but these errors were encountered: