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

[GTK] Add missing spaces for consistent style #996

Closed

Conversation

basilevs
Copy link
Contributor

@basilevs basilevs commented Jan 22, 2024

Add missing space after identifier, but before an opening parenthesis or bracket. The fix is done by replacing all occurrences of \w\( and \w\[ regular expressions. Only GTK Tree and TreeItem files are touched.

This fixes some of style defects mentioned in #903

Rationale: The existing style inconsistencies make it harder to keep consistent style in #918

Add spaces before parentheses and brackets

The defects are mentioned in
eclipse-platform#903
Copy link
Contributor

Test Results

   299 files  ±0     299 suites  ±0   6m 10s ⏱️ +22s
 4 098 tests ±0   4 090 ✅ ±0   8 💤 ±0  0 ❌ ±0 
12 206 runs  ±0  12 133 ✅ ±0  73 💤 ±0  0 ❌ ±0 

Results for commit addf7d8. ± Comparison against base commit 26626af.

@basilevs basilevs changed the title Add missing spaces for consistent style [GTK] Add missing spaces for consistent style Jan 22, 2024
@basilevs
Copy link
Contributor Author

@SyntevoAlex please share your thoughts.

@iloveeclipse
Copy link
Member

  1. I'm not sure this style is default one in Eclipse and SWT, I believe the opposite is the case.
  2. Changing style by hand is error prone, next edit will not consider this special style and we have disconnect again - therefore one should make sure formatter settings are committed for project and that should be followed.
  3. Not sure why only two files are changed.
  4. Git blame will show now your commit for almost every line, because it is not just leading/trailing whitespece change.

In sum : I dislike this change and wish I wouldn't see it 1:(

@basilevs
Copy link
Contributor Author

basilevs commented Jan 22, 2024

  1. I'm not sure this style is default one in Eclipse and SWT, I believe the opposite is the case.

@SyntevoAlex requests this style in #903

2. Changing style by hand is error prone, next edit will not consider **this** special style and we have disconnect again - therefore one should make sure formatter settings are committed for project and that should be followed.

This style is so exotic, that I don't think it can be expressed in Eclipse Formatter terms (and there is no formatter file for SWT to begin with).

  1. Not sure why only two files are changed.

The method (to update the style) I use is not universal and I need only these to files to be consistent to proceed with #918

4. Git blame will show now your commit for almost every line, because it is not just leading/trailing whitespece change.

Sure, just like any other style fix.

In sum : I dislike this change and wish I wouldn't see it 1:(

Same here. One way or another, I need to make the style consistent, so that I could apply my changes in a consistent manner.

@iloveeclipse
Copy link
Member

Same here.

So let's close this PR.

@basilevs
Copy link
Contributor Author

Same here.

So let's close this PR.

Should I then format the files with default Eclipse Formatter? I still need to somehow apply my change for #918, which have to be consistent with something.

@iloveeclipse
Copy link
Member

I haven't checked your patch, sorry. I personally prefer to use "most convenient" coding style for changed code, that a) consistent and b) matches common understanding about format used in the project in question. Also I usually try to separate "cleanup" from "functional" changes.

@basilevs
Copy link
Contributor Author

basilevs commented Jan 22, 2024

I haven't checked your patch, sorry. I personally prefer to use "most convenient" coding style for changed code, that a) consistent and b) matches common understanding about format used in the project in question. Also I usually try to separate "cleanup" from "functional" changes.

The exact content of my patch does not matter (I just need some kind of guidance on style it should be done in). The most convenient style is not consistent with current SWT codebase. And this discussion shows that there is no common understanding. This MR was a separate cleanup change.

I guess, I will try to stick to the exotic style demonstrated here, but will only update changed methods.

@SyntevoAlex
Copy link
Member

I agree that code style in SWT is somewhat exotic.

I would suggest to not touch what already exists, unless you're rewriting most of function's code.
If you're changing something slightly, keep to existing style, whatever it is.

@SyntevoAlex
Copy link
Member

SyntevoAlex commented Jan 24, 2024

In new code, you could pick from any of the styles that are widely represented in SWT.
For example, putting spaces before () are seen in both ways in SWT:
void foo ()
void foo()

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