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

feat: ensure code is formatted and imports are sorted before commit #3381

Merged
merged 2 commits into from
Jan 13, 2025

Conversation

epochcoder
Copy link
Contributor

I have noticed that there are commits (on master) that need to fix formatting and imports occasionally, this is due to authors not having run a mvn clean install on their PR's, even though the builds are successful.

This will help keep master consistent in that it would run a format & sort before commit, even though it makes the commit slightly slower (on my machine 2 seconds), I believe the tradeoff is worth it.

@harawata
Copy link
Member

harawata commented Jan 4, 2025

A little bit scary, but it sounds awesome.
Can it update the license years as well? Or, that one is too slow?

As I have no idea what the implications are, please wait for @hazendaz 's review.

@epochcoder epochcoder marked this pull request as draft January 4, 2025 12:23
@epochcoder
Copy link
Contributor Author

Can it update the license years as well? Or, that one is too slow?

Unfortunately yes, I think we will scream every time we need to commit 🤣

I still need to make one change, the formatted files don't get added to git again, so I will check first if there would have been a failure, then format and sort, and ask the developer to commit again. This is the same principle as pre-commit.com; But i did not want to use that as it has a requirement on python being present. but will for sure wait for @hazendaz

@harawata
Copy link
Member

harawata commented Jan 4, 2025

Damn...I don't even know why it has to be written in each and every file, but there has got be a better way in 21th century. 😞

If there are, automatically sort and format and fail the commit
 as we cannot reliably tell which files to add back to staging
@epochcoder
Copy link
Contributor Author

epochcoder commented Jan 4, 2025

Damn...I don't even know why it has to be written in each and every file, but there has got be a better way in 21th century. 😞

Completely agree, or at least have it not take 8 minutes :)

Anyway, tested it now, and if there are files to be formatted or sorted, it will automatically happen, and the commit will fail, the author can then re-commit the correct files

@epochcoder epochcoder marked this pull request as ready for review January 4, 2025 12:49
@epochcoder epochcoder self-assigned this Jan 4, 2025
@hazendaz
Copy link
Member

hazendaz commented Jan 4, 2025 via email

@epochcoder
Copy link
Contributor Author

@hazendaz Are you ok with this for now, or would you like to add the license config to this PR?

@hazendaz
Copy link
Member

Hi, sorry for delay. I'm actually on the impsort, formatter, and license plugin teams. While license plugin, which you didn't add would kill performance, there is good news coming there. See mathieucarbou/license-maven-plugin#877. There is also a pending pull request over there that we should be able to link in just files that changed far easier than it supports today (it does support it but understood as clunky).

For the other two, I haven't looked in a while, but have you checked to make sure those don't support passing only the files that got changed? If not, that is something I can help make happen.

Here is where those two plugins are located

https://github.com/revelc/impsort-maven-plugin
https://github.com/revelc/formatter-maven-plugin

For reference on impsort and formatter, here are the stats I got on my windows 11 machine with about 3 year old intel i7.

net.revelc.code.formatter:formatter-maven-plugin:2.24.1:format {execution: default} | 3.054 s
net.revelc.code:impsort-maven-plugin:1.12.0:sort {execution: default} | 2.451 s

So they are not bad in general. Once the license plugin gets the next release, probably in a month or so, if it were done exactly the same, a commit would generally take about 30 seconds overall if that were in this mix. Then provided fact it will allow passing in only the changes files and if the others can, I believe we can get that down to simply seconds.

So here is how I would like to approach this.

  • See if the impsort / formatter currently support passing files only to be formatted, if not, please open a ticket on the plugins explaining the need. I seem to recall they probably don't and think there was an ask for that on formatter.
  • If they do already support that, please work out the details in the hook here to provide the changed files in the configuration here.
  • For license, for now, don't add it, we can continue to deal with those even though that is going to be high on the missed commits given we just landed in 2025.
  • I think otherwise, this PR unless @harawata has can concerns simply can be merged as-is with the rest as follow up.

Another thing, I'll do in the 'parent' pom of this project in order to have the same behavior across mybatis repos is setup the caching that impsort / formatter are both already configured to use switched to the m2 repository folder (currently uses target folder). By doing that and fact we already cache the m2 location on github actions, we would for sure get more improvements on those even if not that big of a deal.

@@ -419,6 +420,24 @@
</rules>
</configuration>
</plugin>

<plugin>
Copy link
Member

Choose a reason for hiding this comment

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

I'll pull this up to the mybatis parent before the next release at least in general configuration so other projects can benefit from it as many of the projects already have some of the exact same formatting enabled. I think this plugin can possibly use the hook from a jar and will look into that just in case that is possible so we don't need to repeat any of it. For now this is fine.

Copy link
Member

@harawata harawata left a comment

Choose a reason for hiding this comment

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

Thank you, @epochcoder @hazendaz !
I can't wait for the license plugin update! 😃 🚀

@epochcoder
Copy link
Contributor Author

@hazendaz Good to know there are improvements coming! And that you are on the team :)

I don't think having the changed file input for impsort and formatting would be worth it tbh. They are both already super quick, however if the maintainers believe it is worth the effort, it definitely won't hurt.

For license plugin, it would be definitely worth it, as even with a 30 second pre-commit, that is quite annoying. Anything longer than 10 seconds I believe dev's start to feel it a lot more.

When it does support this though, I think a move to https://pre-commit.com/ makes more sense, as it supports changed files by default and we won't need to write shell scripts; It is also a bit more extensible and configurable.

I'll see if there is an open issue on the licence plugin, or open a new one :)

@epochcoder epochcoder merged commit b762658 into mybatis:master Jan 13, 2025
19 checks passed
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