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

Avoid CPP in Config.hs #315

Closed
fendor opened this issue Oct 31, 2021 · 4 comments
Closed

Avoid CPP in Config.hs #315

fendor opened this issue Oct 31, 2021 · 4 comments
Labels
good first issue Good for newcomers type: enhancement New feature or request

Comments

@fendor
Copy link
Collaborator

fendor commented Oct 31, 2021

In haskell/lsp#360, compatibility with aeson >= 2 is achieved without using CPP, we want that too, because no one likes CPP!

@fendor
Copy link
Collaborator Author

fendor commented Oct 31, 2021

Rewriting should be possible, however, the parsers of Config.hs are hard to follow, so this might be an opportunity to rewrite the parsers.

@fendor fendor added good first issue Good for newcomers type: enhancement New feature or request labels Jan 17, 2022
@ThomasCrevoisier
Copy link
Contributor

👋 I can try to give a shot at this :) Though I would need some pointers before doing so.

From what I see, the only impacted points are parsing HashMap and the use of KeyMap in parseSingleOrMultiple, right ?

however, the parsers of Config.hs are hard to follow, so this might be an opportunity to rewrite the parsers

Do you have something specific in mind ?

@fendor
Copy link
Collaborator Author

fendor commented Feb 12, 2022

Hello! Happy to hear it!

True, I think so as well, these are the only locations where we access the Keys.
To be frank, I am not fully sure right now, whether we can avoid all CPP, but any reduction would be great!

Do you have something specific in mind ?

I feel like we have a number of code smells, for example, we dispatch manually the cradle type in https://github.com/haskell/hie-bios/blob/master/src/HIE/Bios/Config.hs#L158.

Then the individual parsers feel a little off as well, take https://github.com/haskell/hie-bios/blob/master/src/HIE/Bios/Config.hs#L203 where we explicitly check the size of the Aeson object to determine what values are possible and provide a proper error message for the user (e.g. when we fail to parse a hie.yaml). Maybe we can improve this parser? Or can at least generate the same error message without hard-coding the available values?

In general, everything you would consider a code-smell can and should be re-worked!

The structure and code of the types CradleConfig and CradleType can be changed if you think it improves the code-quality.

If you are not sure about something, you can either ask here or ping me in irc/matrix (libera.chat #haskell-language-server, I have the same name)

@fendor
Copy link
Collaborator Author

fendor commented Jan 23, 2023

Closing, has been fixed by #329

@fendor fendor closed this as completed Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers type: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants