Skip to content

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
Member

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
@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