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

also search for .regal.yaml #1288

Closed
grosser opened this issue Dec 4, 2024 · 9 comments · Fixed by #1339
Closed

also search for .regal.yaml #1288

grosser opened this issue Dec 4, 2024 · 9 comments · Fixed by #1339

Comments

@grosser
Copy link

grosser commented Dec 4, 2024

as per https://docs.styra.com/regal#configuration regal looks for .regal/config.yaml
... but I don't really want a whole folder for just 1 file, and rather have another dot-file in the base of the repo as so many other tools do

@anderseknert
Copy link
Member

Hey, and thanks for the feedback!

I don't really have a strong preference on that, so looking for a .regal.yaml (or whatever we'd want the name to be?) in addition to .regal/config.yaml sounds reasonable to me. The reason we went with a directory is that it allows us to use it for more than configuration. Like how custom linter rules can be placed under .regal/rules and automatically be picked up by the linter. But most users probably don't have custom rules, so providing more options for where to put their configuration would be nice.

The --config-file flag can be used with regal lint for this today, but it's obviously not as ergonomic as a default location. And AFAIR, it doesn't work with the language server.

@charlieegan3
Copy link
Member

The language server is using the config detection logic config.FindConfig which walks upwards looking for a .regal dir. In theory, we could have this file look for a .regal.yaml as well while doing that walk, with .regal/config.yaml taking precedence.

Though I prefer the explicit --config-file option in theory, the question is how to support that in the language server. Adding the flag is easy, but we'd need to update clients to support setting the flag - and that seems worse to me than just checking for .regal.yaml.

just my 2c.

@anderseknert
Copy link
Member

The language server is using the config detection logic config.FindConfig which walks upwards looking for a .regal dir. In theory, we could have this file look for a .regal.yaml as well while doing that walk

Sounds good to me!

with .regal/config.yaml taking precedence

I think we should fail with an explanation in case two config files are found, as that's almost certainly a user mistake that they'd better address than having to try and figure out why their config isn't getting loaded.

Adding the flag is easy, but we'd need to update clients to support setting the flag - and that seems worse to me than just checking for .regal.yaml.

I suppose this could be passed in the initOptions or whatever the client sends? But I'd defer that until someone actually asks for that :)

@charlieegan3
Copy link
Member

Ok yeah, the error on on conflict sounds like a better system. I can start on this later today.

charlieegan3 added a commit to charlieegan3/regal that referenced this issue Jan 15, 2025
charlieegan3 added a commit to charlieegan3/regal that referenced this issue Jan 15, 2025
anderseknert pushed a commit to charlieegan3/regal that referenced this issue Jan 15, 2025
charlieegan3 added a commit to charlieegan3/regal that referenced this issue Jan 15, 2025
charlieegan3 added a commit that referenced this issue Jan 15, 2025
* config: Allow loading of .regal.yaml file too

Fixes #1288

Signed-off-by: Charlie Egan <[email protected]>

* config: Choose most specific file

Signed-off-by: Charlie Egan <[email protected]>

* Markdown line length lint

* / -> or

---------

Signed-off-by: Charlie Egan <[email protected]>
@charlieegan3
Copy link
Member

Hey @grosser, this features in the 0.30.1 release!

@grosser
Copy link
Author

grosser commented Jan 16, 2025

thank you! :D

@grosser
Copy link
Author

grosser commented Jan 16, 2025

I think 1 area was missed, created #1341

@charlieegan3
Copy link
Member

Ouch, will look into it!

@grosser
Copy link
Author

grosser commented Jan 22, 2025

another related issue #1342

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 a pull request may close this issue.

3 participants