Skip to content

Depend on haskell-mode #164

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
Merged
merged 2 commits into from
Jan 2, 2023
Merged

Depend on haskell-mode #164

merged 2 commits into from
Jan 2, 2023

Conversation

glyn
Copy link
Contributor

@glyn glyn commented Dec 30, 2022 •
edited
Loading

Note to reviewers: this PR is untested.

Fixes #163

glyn
glyn commented Dec 30, 2022
View reviewed changes
lsp-haskell.el
;; Version: 1.0
;; Package-Requires: ((emacs "24.3") (lsp-mode "3.0"))
;; Version: 1.1
;; Package-Requires: ((emacs "24.3") (lsp-mode "3.0") (haskell-mode "16.1"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

16.1 appears to be current at the time of writing. I'm not really sure what to put here.

glyn
glyn commented Dec 30, 2022
View reviewed changes
Eask
@@ -1,5 +1,5 @@
(package "lsp-haskell"
"1.0"
"1.1"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered 1.0.1, but requiring haskell-mode feels like a significant change so 1.1 or 2.0 is probably more appropriate, depending on the versioning policy in use.

Copy link
Member

Choose a reason for hiding this comment

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

Make sense, but let's wait what other people think! Thanks!

@glyn glyn mentioned this pull request Dec 30, 2022
Closed
@glyn
Copy link
Contributor Author

glyn commented Dec 30, 2022

CI failed:
2022-12-30-184407_905x749_scrot

There seems to be a duplicate dependency on haskell-mode. Not sure what is causing this.

jcs090218
jcs090218 reviewed Dec 30, 2022
View reviewed changes
Copy link
Member

@jcs090218 jcs090218 left a comment

Choose a reason for hiding this comment

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

That should fix the CI, see review.

@glyn
Copy link
Contributor Author

glyn commented Dec 31, 2022

@jcs090218 Please would you approve the workflow so we can see if CI is fixed? Also, kindly PTAL at my other comments in this PR.

@jcs090218
Copy link
Member

Please would you approve the workflow so we can see if CI is fixed?

I don't have permission to this repo; let me ping repo owners. They should be able to help us!

cc @yyoncho @alanz

jcs090218
jcs090218 approved these changes Dec 31, 2022
View reviewed changes
Copy link
Member

@jcs090218 jcs090218 left a comment

Choose a reason for hiding this comment

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

As I mentioned above, I don't have permission, but the code LGTM! :)

Thanks for updating the README as well! ❤️ 👍

@michaelpj michaelpj marked this pull request as ready for review January 2, 2023 19:12
michaelpj
michaelpj approved these changes Jan 2, 2023
View reviewed changes
Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@michaelpj michaelpj merged commit c5d3bc2 into emacs-lsp:master Jan 2, 2023
@glyn
Copy link
Contributor Author

glyn commented Jan 2, 2023

Looks great, thanks!

On the plus side, you're welcome. On the other hand, eek, let's hope it works!

@glyn glyn deleted the 163-depend-on-haskell-mode branch January 2, 2023 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelpj michaelpj michaelpj approved these changes

@jcs090218 jcs090218 jcs090218 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

README missing a step?
3 participants
@glyn @jcs090218 @michaelpj