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

Apply project settings to o.e.osgi.util #324

Merged

Conversation

Torbjorn-Svensson
Copy link
Contributor

  1. Convert to LF and ensure LF at EOF:
find . -type f -print0 | \
  xargs -r0 perl -le 'for (@argv) { print if -f && -T }' | \
  xargs -rd'\n' dos2unix -e

@github-actions
Copy link

github-actions bot commented Oct 1, 2023

Test Results

     24 files  ±0       24 suites  ±0   11m 33s ⏱️ +2s
2 150 tests ±0  2 105 ✔️ +1  45 💤 ±0  0  - 1 
2 194 runs  ±0  2 149 ✔️ +1  45 💤 ±0  0  - 1 

Results for commit 405ec98. ± Comparison against base commit 8beb9ce.

♻️ This comment has been updated with latest results.

@akurtakov
Copy link
Member

@tjwatson As a PL I would like to get your opinion on this(and other similar) mass changes..

@Torbjorn-Svensson
Copy link
Contributor Author

My aim with all these "cleanup" PRs is to make it trivial to do code changes that only contain the intended code change in the diff after applying the automatic formatting rules in the Eclipse IDE.
Right now, if you do a minor change in one of the bundles, the entire file is rewritten and the change is "lost" in the diff.

I have never worked on TCK before. Can you please enlighten me where I can read up on how it works as I can't connect my changes in this PR to how a unit test can fail.

@akurtakov
Copy link
Member

@laeubi Can you reply to the question about TCKs?

@laeubi
Copy link
Member

laeubi commented Oct 2, 2023

Some TCKs are not successful for Equinox, but ti seems there is not much people caring about the full compatibility of edge cases. So if there are no new failures it should be fine:

https://github.com/eclipse-equinox/equinox#implemented-services-an-compliance

beside that, instead of reformatting all code now there are the following alternatives:

  1. before working on a file, reformat it and commit that change, then perform your change and add another commit, in the Github UI it is now possible to only look at that particular diff that changed the code
  2. enable "only format edited files" option to only ever change/reformat the single portion of code where a change was applied.

@Torbjorn-Svensson
Copy link
Contributor Author

Some TCKs are not successful for Equinox, but ti seems there is not much people caring about the full compatibility of edge cases. So if there are no new failures it should be fine:

https://github.com/eclipse-equinox/equinox#implemented-services-an-compliance

Ok, so I assume that PRs that does not fail on any other step than the TCKs are fine.

beside that, instead of reformatting all code now there are the following alternatives:

1. before working on a file, reformat it and commit that change, then perform your change and add another commit, in the Github UI it is now possible to only look at that particular diff that changed the code

To me, this sounds sub-optimal as it would require more work from contributors than just doing the change once and move on with other tasks.

2. enable "only format edited files" option to only ever change/reformat the single portion of code where a change was applied.

I suppose you talk about "only format edited lines" and yes, that could work, but it would make the code much harder to read. Personally, I do not consider this a solution.

@HannesWell
Copy link
Member

To me, this sounds sub-optimal as it would require more work from contributors than just doing the change once and move on with other tasks.

It is, but it is also a lot of work to review all the changes :)

And please note as well, that in some cases Equinox is embedding external sources, e.g. OSGi API (in packages org.osgi*) or from Apache Felix (org.apache.felix*)

@Torbjorn-Svensson
Copy link
Contributor Author

To me, this sounds sub-optimal as it would require more work from contributors than just doing the change once and move on with other tasks.

It is, but it is also a lot of work to review all the changes :)

That's why I included the command that I ran to produce the change :)

And please note as well, that in some cases Equinox is embedding external sources, e.g. OSGi API (in packages org.osgi*) or from Apache Felix (org.apache.felix*)

Ok, so in that case, it's it desired to have them follow the coding rules defined in the bundle or that they break whenever someone tries to correct a small typo or fix a bug in them?
Depending on the reply, I can exclude them from the PR(s) if I just get a list of trees that are embedded from external sources.

@tjwatson
Copy link
Contributor

tjwatson commented Oct 5, 2023

Depending on the reply, I can exclude them from the PR(s) if I just get a list of trees that are embedded from external sources.

org.osgi and org.apache packages should remain unchanged. If there is a bug/type/enhancement needed we go back to the source projects to fix.

@tjwatson
Copy link
Contributor

tjwatson commented Oct 5, 2023

My aim with all these "cleanup" PRs is to make it trivial to do code changes that only contain the intended code change in the diff after applying the automatic formatting rules in the Eclipse IDE.

In the past we had setup project specific settings format on save action -> "Format edited lines" so it does not change the whole file. Perhaps that got lost along the way?

@tjwatson
Copy link
Contributor

tjwatson commented Oct 5, 2023

@tjwatson As a PL I would like to get your opinion on this(and other similar) mass changes..

We should only do this for org.eclipse source of the project (not org.osgi or org.apache` source). We should make sure the project settings are set to have "Format edited Lines" only on the save action.

I'm all for converting to LF and ensuring EOF though (for org.eclipse source). I thought we did that along time ago.

@Torbjorn-Svensson
Copy link
Contributor Author

@tjwatson As a PL I would like to get your opinion on this(and other similar) mass changes..

We should only do this for org.eclipse source of the project (not org.osgi or org.apache` source). We should make sure the project settings are set to have "Format edited Lines" only on the save action.

I'm all for converting to LF and ensuring EOF though (for org.eclipse source). I thought we did that along time ago.

I've updated the PRs to not include what I believe is org.osgi and org.apache sources.

Let me know if there is something else I missed in the PRs.

This was achieved by running:
find . -type f -print0 | \
  xargs -r0 perl -le 'for (@argv) { print if -f && -T }' | \
  xargs -rd'\n' dos2unix -e

Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@st.com>
@akurtakov akurtakov merged commit acde0b3 into eclipse-equinox:master Nov 4, 2023
22 of 25 checks passed
@Torbjorn-Svensson Torbjorn-Svensson deleted the pr/cleanup-o.e.osgi.util branch November 5, 2023 10:12
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.

5 participants