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

Improve format check #4096

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

atakavci
Copy link
Contributor

@atakavci atakavci commented Feb 21, 2025

This is an improvement to our iterative approach for code style checks.

Issue Statement
The Java Format Check flow fails in any pull request (PR) unless all modified Java files are properly formatted. The check evaluates the entire file, not just the lines that were changed or added. This means that even if the change is a single line, the entire file is checked, and any formatting issues in untouched parts of the file will cause the check to fail. As a result, the PR may be marked as failed at the higher level, even if only the unmodified sections of the file contain formatting issues. This is undesirable, as it makes the PR stand out as a "checks failed", in the list of PRs and so on,, just because of the formatting issues that aren’t part of the actual change.

Suggested Improvement
We propose to check and compare the number of formatting issues at the file level for each modified file. If the number of formatting issues changes (i.e., new issues are introduced or existing ones are fixed), only then will the check fail.

@atakavci atakavci self-assigned this Feb 22, 2025
@atakavci atakavci requested review from tishun and ggivo February 22, 2025 12:13
@atakavci atakavci marked this pull request as ready for review February 22, 2025 12:14
@tishun
Copy link
Contributor

tishun commented Feb 28, 2025

Hey sorry if this sounds too rash but ... I am really not sure why we are wasting time thinking about this when we can simply reformat the repository and be done with it.

Ultimately we do want to have consistent formatting, correct?
Why do we postpone having one?

@ggivo
Copy link
Collaborator

ggivo commented Mar 4, 2025

@atakavci
I took a look, and I think with more complex reviews or just adding a couple of lines will end up in the same situation (e.g it will report an error).

I agree with @tishun that we just need to reformat the whole code base at once.
I am just wondering when will be the correct time to do it. Do you think it makes sense to wait for 6.0?

@atakavci
Copy link
Contributor Author

atakavci commented Mar 4, 2025

from my POV, its all about the impact of it on other branches and forks with open PRs .
for sure we can schedule for a single shot change, though we can find a time which would be only correct for ourselves. jedis has 4K forks.
if you feel ok with it, i dont have reason against waiting 6.0.

@atakavci
Copy link
Contributor Author

atakavci commented Mar 4, 2025

I took a look, and I think with more complex reviews or just adding a couple of lines will end up in the same situation

right,, we need something like check numbers and fail only if it increases.

@atakavci
Copy link
Contributor Author

atakavci commented Mar 4, 2025

i agree this is getting complex. TBH, i am surprised that i cant spot a tool/action doing this.
yet still,, i wouldn't do it one shot.
leaving this up to you to decide, let me know if i can help.

@tishun
Copy link
Contributor

tishun commented Mar 7, 2025

I can understand your hesitation. Such a complex change is bound to hurt somebody. But in my opinion this is fine.

We need to "bite the bullet" and draw the line if we want to have consistent formatting.
All other solutions are bound to be complex and hard to maintain / enforce.

Jedis forks

We have no obligation to keep our codebase compatible with other forks, it is the other way around.
If the code has deviated too much and this becomes a burden the maintainer of the fork should make a decision on how to proceed, but we can't base our decisions on the fact that there are hundreds or thousands of forks of the repo.

Jedis branches

This is the problem I personally feel is going to have the most impact on us. This is why I said "bite the bullet".
If we think there is a high probability that many new changes need to be ported back then this is nota good time.
In a normal situation we should have a few (in a matter of tens) reviews that would not be an easy crossport, due to formatting. But even then a manual merge would be relatively easy as there is no actual code change.

So my opinion is to reformat the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants