>by simply running `gradle spotlessApply`. It was trivially easy to fix, and anybody could have done it.
but that requires "anybody" to have gradle setup on their dev machine, and not everybody is java/jvm developer with gradle setup :) yes for people contributing java/kotlin PR that breaks build they *probably* already have gradle setup, but there are still exceptions (like me trying to fix the failed build just by looking at the code without gradle setup). that's not always true *after* the broken code is already merged and someone just tries to fix the build. and I think I've seen it at least a few times that the github dependency bot tries to upgrade the spotless plugin to a new version which would start to fail the code that passed before for style reasons. so people need to either reject the PR, or have gradle setup and run gradle to fix the code that's starting to break before merging the dependency PR, which I think essentially reduces the pool of maintainers to response to those dependency PRs (while our number is already not great). what do you think if we keep the spotless and ktfmt steps, but add `continue-on-error: true` ( https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#jobsjob_idstepscontinue-on-error)? if the code truly not builds (like the recent commit) it would still fail the later steps on the CI, but if it's purely style issue, that at least allows us to run cross-tests? On Wed, Aug 14, 2024 at 2:37 PM Christopher <ctubb...@apache.org> wrote: > I strongly disagree with this. spotless doesn't just standardize > formatting, but also checks for a wide variety of code quality issues > that people should address. Just because the check is getting ignored > and not fixed right away doesn't mean the solution is to remove the > check. It means more attention needs to be paid to fix what the check > is failing. > > Also, spotless depends on compilation step. It's just the entry point > into the build, not just a formatting check. If you remove the > spotless check without adding a compilation step, then no checks are > done at all that verify the code even builds. > > Also, it's very easy for anybody to fix most spotless checks. > > The real problem is that people are accepting PRs and merging them > when the PR fails the check. The person who made the edit that caused > the breakage should fix their PR before accepting it. This isn't just > a problem for formatting/style issues. This is a problem with basic > compilability. Recently a commit got into the Java code that didn't > even compile at all, and ended up needing a new PR to fix it. If the > build failure were attended to instead of ignored, that wouldn't have > happened. > > I trivially fixed the spotless check this afternoon in > > https://protect.checkpoint.com/v2/___https://github.com/apache/thrift/pull/3019___.YzJ1OnJlZGRpdDpjOmc6NWYxODdjYjFkYmUzM2IzNjI4NmRhNDdiM2ZhMWZmYWU6NjpjZDdjOmQ5N2ZhNzBlNDdiZjRiNWVhOWUyYjRkMDRlMWJhOWQ1YzM5NTRiYzBjZTJjYWJkY2IxODZhZGViYjNmMDhmNGU6cDpUOk4 > by simply running `gradle > spotlessApply`. It was trivially easy to fix, and anybody could have > done it. > > I'm a strong "no" against removing the spotless check. > > On Wed, Aug 14, 2024 at 4:53 PM Yuxuan Wang > <yuxuan.w...@reddit.com.invalid> wrote: > > > > I created > https://protect.checkpoint.com/v2/___https://github.com/apache/thrift/pull/3020___.YzJ1OnJlZGRpdDpjOmc6NWYxODdjYjFkYmUzM2IzNjI4NmRhNDdiM2ZhMWZmYWU6Njo4MmZkOjQ2MjYxNWYyYjdiN2NlYWRhMjgzOGMxNTUxMThkZTFkMTg3ODBkMGI3MzQ1ZmE4MGUwOTMyOTY0ZjYyZGExZTI6cDpUOk4 > as a test to see how > > that works. > > > > That PR also includes changes in > https://protect.checkpoint.com/v2/___https://github.com/apache/thrift/pull/3019___.YzJ1OnJlZGRpdDpjOmc6NWYxODdjYjFkYmUzM2IzNjI4NmRhNDdiM2ZhMWZmYWU6NjpjODhkOjE3MWQ2ZTc3YTQyZDlhZmQzMTE3ZDc5ZWFjNDI5NzIwN2U5MjczYmIyOGE5Nzc5MzI0MDNmZGY4ZjJjMGNkNTk6cDpUOk4 > , > > as otherwise the java lib code won't even build. Which I think shows > > another issue with the spotless check currently: it hides bigger problem > > that could cause the java lib to stop building, as when spotless check > > fails we stops CI early and won't know whether the java lib builds. > > > > On Wed, Aug 14, 2024 at 9:42 AM Yuxuan Wang <yuxuan.w...@reddit.com> > wrote: > > > > > From my observation of the past few years, every time the github > > > dependency bot upgrades the spotless plugin version, it will start to > fail > > > something that passed before. And it usually takes months for someone > to > > > fix those, if they are ever fixed. > > > > > > While having some enforcement on styling is great, making those > > > enforcements to break builds (and also block cross tests from running) > for > > > months is not. If we don't have the resources to keep the styles > > > up-to-date, then maybe sacrificing some styling in favor of actual > cross > > > tests is a better approach? > > > >
