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

Re-add hls-stan-plugin #3851

Merged
merged 8 commits into from
Nov 19, 2023
Merged

Re-add hls-stan-plugin #3851

merged 8 commits into from
Nov 19, 2023

Conversation

0rphee
Copy link
Collaborator

@0rphee 0rphee commented Oct 24, 2023

Hi! now that stan supports ghc-9.6 (kowainik/stan#523), I figured that it would be possible to add its hls plugin back.

I tested it locally with 9.6.3.

9.8.1 support for the stan plugin tests requires to add th-abstraction to allow-newer in cabal.project, and depends on the publishing of 9.8.1 support to hackage for hiedb. (They are indirect dependencies, I'm not sure which dependency might trigger them, I'm not used to cabal)

If something more is needed, please tell me and I'll see what I can do :)

img

cabal.project Outdated
./plugins/hls-gadt-plugin
./plugins/hls-explicit-fixity-plugin
./plugins/hls-explicit-record-fields-plugin
./plugins/hls-refactor-plugin
./plugins/hls-overloaded-record-dot-plugin

index-state: 2023-10-06T06:12:29Z
-- index-state: 2023-10-06T06:12:29Z
Copy link
Collaborator Author

@0rphee 0rphee Oct 24, 2023

Choose a reason for hiding this comment

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

I commented out the index-state to get the new stan version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You should just update the index-state. No point in commenting it out and the PR can't be merged without it any way.

@0rphee 0rphee changed the title Re-add hls-stan-plugin from Re-add hls-stan-plugin Oct 24, 2023
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

While it is great to have stan back, perhaps even with new diagnostics, we lack a maintainer for the plugin. Are you volunteering to maintain the stan plugin in the foreseeable future? If so, please add yourself to the CODEOWNERS file.

Since we do experience some issues with plugins that bitrot because it has no active maintainer, I don't think we will merge this until we have found a maintainer.

@0rphee
Copy link
Collaborator Author

0rphee commented Oct 29, 2023

Maybe i could do it. What is expected of a maintainer? I suppose mainly keep it running with ghc changes and fix bug reports?

@fendor
Copy link
Collaborator

fendor commented Oct 29, 2023

Exactly, and you will mainly be pinged in the case of hls-stan-plugin related issues, but it does not include fixing the bug upstream in stan.

Fix ghc bounds for stan plugin

Update index-state
@0rphee
Copy link
Collaborator Author

0rphee commented Oct 30, 2023

Ok. I will mantain the plugin, I've added myself to the CODEOWNERS file.

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM, thank you very much for becoming the maintainer of the plugin!
Once the PR is approved, we will invite you to the repo as a contributor, then the Codeowners file will not display any errors any more.

docs/support/plugin-support.md Outdated Show resolved Hide resolved
@fendor fendor added the status: needs review This PR is ready for review label Oct 31, 2023
@michaelpj
Copy link
Collaborator

I wonder if @tomjaguarpaw would also be interested in being involved?

@tomjaguarpaw
Copy link
Member

Hi, thanks for pinging me. I don't have the bandwidth to be a maintainer of the plugin myself, but I'm happy to unblock any issues that need help on the upstream stan side.

@fendor
Copy link
Collaborator

fendor commented Nov 15, 2023

@0rphee You received an invitation to this repo! Once you have accepted, we will merge this PR :) Thank you for your patience!

@michaelpj
Copy link
Collaborator

(and please fix the conflicts)

@0rphee
Copy link
Collaborator Author

0rphee commented Nov 18, 2023

I've accepted the invitation to the repo :)

The conflicts are fixed. From what I've seen, the stack lts21 previous build failure was caused by a now fixed issue (#3868). Also, I now remembered about the hls-stan-plugin test suite (I forgot to add it to the gh actions workflow), should I still add it back?

@fendor
Copy link
Collaborator

fendor commented Nov 18, 2023

The conflicts are fixed. From what I've seen, the stack lts21 previous build failure was caused by a now fixed issue (#3868).

🎊

Also, I now remembered about the hls-stan-plugin test suite (I forgot to add it to the gh actions workflow), should I still add it back?

Ah, yes please, add it back to the GitHub actions. Otherwise, we won't run the test suite in CI!

@0rphee
Copy link
Collaborator Author

0rphee commented Nov 18, 2023

I think everything is fine now!

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM!

@fendor fendor merged commit 2764d04 into haskell:master Nov 19, 2023
44 checks passed
@tomjaguarpaw
Copy link
Member

This is great to see! Thanks @0rphee.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs review This PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants