-
Notifications
You must be signed in to change notification settings - Fork 62
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
Refactor parsing of hie.yaml files #329
Refactor parsing of hie.yaml files #329
Conversation
Hi! Since CI has a failure, can you add the failing hie.yaml to the parser-tests? If I read this correctly, should be this one: https://github.com/haskell/hie-bios/blob/master/tests/projects/failing-cabal/hie.yaml EDIT: also rather embarrassing, we forgot to test the trivial case in the parser tests. |
@fendor all should be good now ! |
Unfortunately, the generic variant suffers currently from the issue of ignoring unknown keys. Example: cradle:
cabal:
comp:
- "exe:hie-bios" is an invalid and currently hie-bios reports:
but with this patch, this becomes:
It successfully parses, and ignores that you mistyped or misnamed the argument. Can we do better than that? |
Oh I see. I've got few questions on this point. As I've seen in the test suite, empty objects are allowed right ? Regarding unknown keys, what should be the behavior when the object we provide contains a valid config but with extra keys that we will ignore ? Failing on missing keys is pretty straightforward, but failing on extra unwanted keys might be a bit trickier to do. |
Until now, we threw an error message. The point being that you notice you didn't write the correct configuration and something is not right. At the moment, I think it is better if we throw an error if there are extra keys we don't know. It is indeed trickier :/ |
f2f16ec is a proposal for fixing this particular point. The downside is that we have to duplicate a bit the keys between parsing and checking but the implementation is relatively simple. Let me know what you think ! |
BTW, I did not forget, just didn't have time for an in-depth review (since this is important to get right). So far it looks good, and I am going to review in depth until the end of the weekend. |
Sure, absolutely no worries ! I won't be responsive this week-end, but I'll remain available to work on it next week :) |
This will make it hard to introduce new optional keys, because older HLS versions will now choke on that config. I think a warning is better.
|
I see your point and did not consider this angle before. @ThomasCrevoisier Would it be hard to turn these errors into warnings? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I don't want to keep you waiting, we are going to merge this as it is an improvement of the status-quo. We will create an issue for the remaining issue
👋 Truely sorry @fendor, those past months have been quite busy and I forgot to come back to this PR 😞 Feel free to move forward with it (I don't have any simple solution in mind regarding extra keys handling unfortunately), and again, sorry for the lack of reactivity. |
No need to apologise, life can be stressful (speaking from experience), and we all deeply appreciate the time you have spent on this! |
Takes care of #315
This introduces a
Hie.Bios.Config.YAML
module which defines datatypes a bit closer to the YAML structure.It allows writing simpler FromJSON instances (without using CPP !) and should be equivalent to the types existing in the
Hie.Bios.Config
module.