search

eclipse-lsp4j / lsp4j

A Java implementation of the language server protocol intended to be consumed by tools and language servers implemented in Java.
https://eclipse.org/lsp4j
Other
613 stars 145 forks source link

Update Completion for 3.17 #582

Closed KamasamaK closed 2 years ago

KamasamaK commented 2 years ago

Collecting all of the Completion-related changes for LSP 3.17.

pisv commented 2 years ago

The changes themselves look great to me, but FWIW https://www.eclipse.org/projects/handbook/#resources-commit states that it is required to use the author’s actual (legal) name and email address in git commit records.

Also, please be aware that using the Squash and Merge button can change author details, as described in https://github.com/isaacs/github/issues/1368. This might get particularly problematic when PR authors prefer to hide their actual name and/or email address on GitHub. A recent example in the lsp4j repository is c75a1a5d563e8b1d792f701636e28145eb2aaa74, which resulted from squashing-and-merging PR #581 containing a number of well-formed commits...

pisv commented 2 years ago

The changes themselves look great to me, but FWIW https://www.eclipse.org/projects/handbook/#resources-commit states that it is required to use the author’s actual (legal) name and email address in git commit records.

Also, please be aware that using the Squash and Merge button can change author details, as described in isaacs/github#1368. This might get particularly problematic when PR authors prefer to hide their actual name and/or email address on GitHub. A recent example in the lsp4j repository is c75a1a5, which resulted from squashing-and-merging PR #581 containing a number of well-formed commits...

I stand corrected -- there is an exception to the rule requiring the author's actual name and email address in git commit metadata. For details, see https://gitlab.eclipse.org/eclipse/dash/org.eclipse.dash.handbook/-/merge_requests/1 and https://www.eclipse.org/lists/eclipse.org-architecture-council/msg04183.html. I was not aware of that. My apologies for the false alarm.

KamasamaK commented 2 years ago

Either way, I've amended the commit. I will do the same for my other recent pending PRs. This was a new clone and I'd forgotten to update the user config.

Regarding squashing, I didn't think there would be an issue. I saw Jonah and yourself even say that it was your preference.

pisv commented 2 years ago

Either way, I've amended the commit. I will do the same for my other recent pending PRs. This was a new clone and I'd forgotten to update the user config.

I see. Thank you!

Regarding squashing, I didn't think there would be an issue. I saw Jonah and yourself even say that it was your preference.

I didn't think there would be an issue either. I just discovered it by chance. That's how the alarm started, actually. However, given the exception to the 'real name/email address' rule, it seems safe to use the Squash and Merge button.