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.

Sign up for GitHub

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

Jump to bottom

Extract an hls-brittany-plugin package #1422

Merged
merged 10 commits into from
Mar 8, 2021
Merged

Extract an hls-brittany-plugin package #1422

merged 10 commits into from
Mar 8, 2021

Conversation

pepeiborra
Copy link
Collaborator

@pepeiborra pepeiborra commented Feb 21, 2021 •
edited
Loading

This also shows how to set up an isolated test suite /cc @konn @isovector @tittoassini see 4c2011a

@pepeiborra pepeiborra force-pushed the brittany-package branch 3 times, most recently from af0dda4 to cccbe4b Compare February 21, 2021 15:08
@jneira
Copy link
Member

jneira commented Feb 21, 2021

This look really great. A minor nitpick: why are you dropping the AGPL flag? brittany continue using AGPL afaics: https://hackage.haskell.org/package/brittany-0.13.1.0/src/LICENSE

@pepeiborra
Copy link
Collaborator Author

pepeiborra commented Feb 21, 2021 •
edited
Loading

The Brittany provider is no longer part of HLS, and there is no direct dependency on Brittany anymore.

If a user doesn't want to acquire a transitive dependency in Brittany, they can disable the 'brittany' flag right?

@jneira
Copy link
Member

jneira commented Feb 21, 2021

yeah, I guess the flag could make more sense if you have several libraries sharing a gpl license and you want to disable all of them at once

@pepeiborra
Copy link
Collaborator Author

This is ready to merge, unless the AGPL flag is needed back

jneira
jneira approved these changes Feb 22, 2021
View reviewed changes
Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

LGTM. no need for AGPL for now imo

@jneira jneira added the merge me Label to trigger pull request merge label Feb 22, 2021
@Ailrun Ailrun removed the merge me Label to trigger pull request merge label Feb 23, 2021
Ailrun
Ailrun approved these changes Feb 23, 2021
View reviewed changes
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.

Thank you for the extraction and for providing an example! I will check the class plugin too.

@pepeiborra
Copy link
Collaborator Author

It looks like the following test fails randomly:

ghcide
  parsedResultAction plugin: FAIL (4.01s)

Has anyone else seen this?

@Ailrun
Copy link
Member

Ailrun commented Feb 23, 2021 •
edited
Loading

It looks like not just that test, but many others suffer nondeterministic "Exception: Language server unexpectedly terminated".

I posted #1430 for the issue.

@haskell haskell deleted a comment from mergify bot Feb 24, 2021
@jneira jneira mentioned this pull request Feb 24, 2021
Closed
@Ailrun Ailrun added the merge me Label to trigger pull request merge label Feb 28, 2021
@pepeiborra pepeiborra merged commit 6fbc602 into master Mar 8, 2021
This was referenced Mar 22, 2021
Merged
Closed
@gustavoavena gustavoavena mentioned this pull request May 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ailrun Ailrun Ailrun approved these changes

@wz1000 wz1000 wz1000 left review comments

@berberman berberman berberman approved these changes

@jneira jneira jneira approved these changes

Assignees
No one assigned
Labels
merge me Label to trigger pull request merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pepeiborra @jneira @Ailrun @wz1000 @berberman