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

Fix VM plugins #2834

Merged
merged 2 commits into from
Sep 5, 2024
Merged

Fix VM plugins #2834

merged 2 commits into from
Sep 5, 2024

Conversation

Wild-W
Copy link
Contributor

@Wild-W Wild-W commented Aug 29, 2024

closes #2831

This pull request fixes VM.OnCompileFunctionParam and its test case.

I had the StyLua autoformatter on so some formatting changes were made to compiler.lua. I'm not sure if this is a problem, as I had just assumed this project was StyLua compliant and I was expected to have the formatter running.

@CppCXY
Copy link
Collaborator

CppCXY commented Aug 30, 2024

I had the StyLua autoformatter on so some formatting changes were made to compiler.lua. I'm not sure if this is a problem, as I had just assumed this project was StyLua compliant and I was expected to have the formatter running.

This assumption is incorrect. We do not use StyLua. Do not use a formatter to make unnecessary changes. You can use range formatting on the code you wrote (if your plugin supports it). From the formatting results, it seems you didn't use StyLua but rather the built-in formatting functionality of LuaLS, as these alignments are unique to LuaLS.

@tomlau10
Copy link
Contributor

And when "fixing up" those unnecessary changes, please use amend/squash/fixup & force push. (or just mix reset everything and recommit from the start)

Otherwise if you just create another commit say fix style issue, this will DOUBLE THE DAMAGE as changes are made TWICE and will leave 2 times changes in the git history. This will greatly impact future usage of git blame (when tracing which changes cause problems while debugging) ☹️


Just a tip for quickly squashing the fixup commit: 😄

  • I use git fork which has a very user-friendly ui: https://git-fork.com/
  • now fix style issue and make a temp commit say fixup
  • then interactive rebase on the parent commit of your pr
    • drag you fixup commit just above your first commit, we are going to combine them
    • change its action to fixup (which is same as squash, except without combining the commit msg)
    • start rebase
  • done 👍 (and should recheck whole the changes again)

@Wild-W
Copy link
Contributor Author

Wild-W commented Aug 30, 2024

And when "fixing up" those unnecessary changes, please use amend/squash/fixup & force push. (or just mix reset everything and recommit from the start)

Otherwise if you just create another commit say fix style issue, this will DOUBLE THE DAMAGE as changes are made TWICE and will leave 2 times changes in the git history. This will greatly impact future usage of git blame (when tracing which changes cause problems while debugging) ☹️

Just a tip for quickly squashing the fixup commit: 😄

  • I use git fork which has a very user-friendly ui: https://git-fork.com/

  • now fix style issue and make a temp commit say fixup

  • then interactive rebase on the parent commit of your pr

    • drag you fixup commit just above your first commit, we are going to combine them
    • change its action to fixup (which is same as squash, except without combining the commit msg)
    • start rebase
  • done 👍 (and should recheck whole the changes again)

Thanks for the tips. I just force reset and recommitted, though your suggestions sound a fair bit more robust. 😄

@Wild-W
Copy link
Contributor Author

Wild-W commented Aug 30, 2024

Oops! I just noticed there were a couple unnecessary formats in plugin.lua too. Should be all good now.

@sumneko sumneko merged commit 08dd0ca into LuaLS:master Sep 5, 2024
10 of 11 checks passed
@sumneko
Copy link
Collaborator

sumneko commented Sep 5, 2024

Thank you!

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.

VM.OnCompileFunctionParam doesn't function as documented
4 participants