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

CI test for emacs-lsp packages #2678

Merged
merged 38 commits into from
Mar 6, 2021
Merged

CI test for emacs-lsp packages #2678

merged 38 commits into from
Mar 6, 2021

Conversation

jcs090218
Copy link
Member

This if for #2655

@jcs090218 jcs090218 added the CI label Mar 6, 2021
@jcs090218 jcs090218 self-assigned this Mar 6, 2021
@jcs090218
Copy link
Member Author

@jcs090218 I couldn't find anything related to lsp-dart, but we do have a Notification protocol not used here 🤔,

Yeah, this looks strange to me. But I sometime do get this warning. See if I can reproduce this. 😕

@yyoncho
Copy link
Member

yyoncho commented Mar 6, 2021

Error is here, https://github.com/emacs-lsp/lsp-mode/runs/2046252831.

The issue is that we have 2 lsp-make-notification functions, one in lsp-mode.el and one in lsp-protocol.el, we should remove the one in lsp-mode.el

@jcs090218
Copy link
Member Author

The issue is that we have 2 lsp-make-notification functions, one in lsp-mode.el and one in lsp-protocol.el, we should remove the one in lsp-mode.el

I could not find the second lsp-make-notification in lsp-protocol.el. Is the line generates it's function?

`(cl-defun ,(intern (format "lsp-make-%s" (s-dashed-words (symbol-name interface))))

I think I should delete the one in lsp-mode?

@ericdallo
Copy link
Member

ericdallo commented Mar 6, 2021

@jcs090218 I think @yyoncho meant https://github.com/emacs-lsp/lsp-dart/blob/master/lsp-dart-protocol.el#L57

I think we can rename the function on lsp-mode or rename on dart side, I don't mean to remove that on Dart side as is not being used ATM, I just added to follow the spec

@yyoncho
Copy link
Member

yyoncho commented Mar 6, 2021

@jcs090218 I think @yyoncho meant https://github.com/emacs-lsp/lsp-dart/blob/master/lsp-dart-protocol.el#L57

I think we can rename the function on lsp-mode or rename on dart side, I don't mean to remove that on Dart side as is not being used ATM, I just added to follow the spec

Lets remove the dart side.

@yyoncho
Copy link
Member

yyoncho commented Mar 6, 2021

But generally, we should prefix the non-core interfaces.

@ericdallo
Copy link
Member

I'll remove it and later add the prefix, it makes sense @yyoncho

@ericdallo
Copy link
Member

Done on master @jcs090218, it should be available on MELPA in 1hour

Makefile Outdated Show resolved Hide resolved
test/test-packages.el Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
test/test-packages.el Outdated Show resolved Hide resolved
@jcs090218
Copy link
Member Author

All checks have passed! 🎉 Let me know if I still need to change anything! ;)

@ericdallo
Copy link
Member

LGTM, thank you for that @jcs090218 :)

test/test-packages.el Outdated Show resolved Hide resolved
@yyoncho
Copy link
Member

yyoncho commented Mar 6, 2021

Looks good to me! I think this is a great addition to our infrastructure. Now it will be much easier to perform cleanup.

@jcs090218
Copy link
Member Author

jcs090218 commented Mar 6, 2021

All checks have passed (again)! Let me merge this for now before this PR get larger and larger. I would apply another patch if needed! :)

Thanks for anyone who reviewed this PR. 🎉

@jcs090218 jcs090218 merged commit b57a9bb into emacs-lsp:master Mar 6, 2021
@yyoncho yyoncho mentioned this pull request Mar 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants