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

Sanitize the setup of the default Ide.Config #1361

Merged
merged 7 commits into from
Feb 14, 2021
Merged

Conversation

pepeiborra
Copy link
Collaborator

@pepeiborra pepeiborra commented Feb 13, 2021

This adds a field in the defaultMain driver to specify the default config, replacing two callbacks and making things a bit saner.

The rest of the changes are to deal with the use of def in various places which had to be turned into a parameter.

Copy link
Member

@Ailrun Ailrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Ailrun
Copy link
Member

Ailrun commented Feb 13, 2021

It looks like HLint plugin requires some changes too.

@pepeiborra
Copy link
Collaborator Author

It looks like HLint plugin requires some changes too.

there isn't a sensible way to fix it, so I just plugged in the type error and let the lsp 1.0 PR take care of it

@pepeiborra pepeiborra added the merge me Label to trigger pull request merge label Feb 13, 2021
@pepeiborra
Copy link
Collaborator Author

@jneira I'm dropping the "ghcide does not support update config" test. Shout if you want to keep it

@wz1000
Copy link
Collaborator

wz1000 commented Feb 13, 2021

@pepeiborra that test is already dropped in my lsp-1.0 PR.

@jneira
Copy link
Member

jneira commented Feb 13, 2021

I will not miss it, thanks for cleaning it out

@pepeiborra
Copy link
Collaborator Author

This keeps failing with:

image

@mergify mergify bot merged commit 57b78e7 into master Feb 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants