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

make path names UNIX-friendly #1460

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

Conversation

elsazac
Copy link
Member

@elsazac elsazac commented Jan 3, 2024

remove white spaces from path names

Copy link
Contributor

github-actions bot commented Jan 3, 2024

Test Results

   930 files  +  310     930 suites  +310   50m 15s ⏱️ + 24m 2s
 7 474 tests ±    0   7 320 ✅  -     1  153 💤 ±  0  1 ❌ +1 
23 571 runs  +7 857  23 059 ✅ +7 664  511 💤 +192  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 740e525. ± Comparison against base commit ad1dfcd.

♻️ This comment has been updated with latest results.

@iloveeclipse
Copy link
Member

Could you please explain what kind of issue you are trying to fix here?
Unix has no issues with space in paths, obviously we build on Mac & Linux without problems.

@elsazac
Copy link
Member Author

elsazac commented Jan 5, 2024

changes in a nutshell:

  • remove white spaces from path names of
    • bundles/org.eclipse.ui.workbench/Eclipse UI
    • bundles/org.eclipse.search/new search
    • bundles/org.eclipse.ui.workbench/Eclipse UI Editor Support
  • adjusted the paths in build.properties

@elsazac
Copy link
Member Author

elsazac commented Jan 5, 2024

Could you please explain what kind of issue you are trying to fix here? Unix has no issues with space in paths, obviously we build on Mac & Linux without problems.

when I work on command prompt for pattern searching I don't get comprehensive results
for eg:

find . -name “*.java” | xargs grep “foo
grep: ./bundles/org.eclipse.search/new: No such file or directory

@laeubi
Copy link
Contributor

laeubi commented Jan 5, 2024

Here it is claimed that either the script or the program should be considered buggy then:

https://unix.stackexchange.com/questions/148043/is-space-not-allowed-in-a-filename

Just in case, Eclipse contains a "search in files" that is much more useful than traditional grep (for me)...

@merks
Copy link
Contributor

merks commented Jan 5, 2024

I've also been bitten by spaces breaking scripts. Of course spaces are not wrong, but they are annoying. And Eclipse search is of course nicer, but not all files are actually visible in the workspace so I too on occasion want to search everything in all the clones...

@akurtakov
Copy link
Member

I've also been bitten by spaces breaking scripts. Of course spaces are not wrong, but they are annoying. And Eclipse search is of course nicer, but not all files are actually visible in the workspace so I too on occasion want to search everything in all the clones...

I can only second that! Nothing is broken in the current state but with this PR certain things would become easier.

@elsazac
Copy link
Member Author

elsazac commented Jan 5, 2024

Here it is claimed that either the script or the program should be considered buggy then:

https://unix.stackexchange.com/questions/148043/is-space-not-allowed-in-a-filename

Just in case, Eclipse contains a "search in files" that is much more useful than traditional grep (for me)...

for example , I was trying to get this pattern for one of my PRs and I was never able to get it straight with the eclipse search

find . -name "*.java" | xargs grep " = " | grep "new" | grep "<" | grep -v "<>"

its possible that it can me made to work just that I am more comfortable in command prompt in scenarios like this.

@laeubi
Copy link
Contributor

laeubi commented Jan 5, 2024

for example , I was trying to get this pattern for one of my PRs and I was never able to get it straight with the eclipse search

I'm not that familiar with grep (that's probably because I need an IDE to work ;-)) but if I decode this correctly you want:

grafik

I am more comfortable in command prompt in scenarios like this

Fair enough, given that this gives me hits in literally thousands of places I personally would be really frustrated in matching the results back to my IDE doing the necessary changes, just assuming you want to replace it afterwards, you can even hit "replace" and get a nice preview if you want and so on...

@elsazac
Copy link
Member Author

elsazac commented Jan 22, 2024

Is there anything that I can do to make progress on this PR? Thanks .

@merks
Copy link
Contributor

merks commented Jan 22, 2024

I think this change is a good idea. Obviously it needs to be rebased.

@iloveeclipse @HeikoKlare @akurtakov

I believe no one objects.

@merks
Copy link
Contributor

merks commented Jan 22, 2024

With so many changes, it's hard to tell if you changed references in the poms, e.g.,

image

@akurtakov
Copy link
Member

It would be nice if search and workbench changes are split to make it easier to look at changes in a single PR.

@iloveeclipse
Copy link
Member

It would be nice if search and workbench changes are split to make it easier to look at changes in a single PR.

If it would be a PR per bundle, it would be easier to review & approve.

@HeikoKlare
Copy link
Contributor

I also appreciate this change, as I also find whitespaces in paths annoying, even though being valid.

Just one consideration concerning the proposed solution: if all these paths are touched anyway, it might make sense to not only remove the whitespaces in the source folder names but to also re-evaluate if the names are good at all. I have not seen many plug-ins with source folders other than src, but those few having multiple source folders (including search and workbench) use very individual names for these folder. If there is no good reason for these special names (such as people being used to them after all the years of existance), I would propose to streamline them in terms of

  1. making them completely lowercase (instead of, e.g., EclipseUIEditorSupport), and
  2. maybe even prefixing them with src to make them easily identifyable as source folder (particularly outside the IDE).

One specific question regarding the workbench plug-in: are these two source folders (Eclipse UI and Eclipse UI Editor Support) necessary at all? After having taken a quick look, I cannot find any configuration requiring this separation (MANIFEST.MF, build.properties).

@merks
Copy link
Contributor

merks commented Jan 22, 2024

I agree with @HeikoKlare that this seems kind of pointless for the org.eclipse.ui.workbench. In this case it looks like some stalled effort (from 2002) described as "First cut of org.eclipse.ui split".

@elsazac elsazac marked this pull request as draft January 22, 2024 09:50
@elsazac
Copy link
Member Author

elsazac commented Jan 22, 2024

Thanks all for the suggestions. Keeping it draft until i rework on this. Meanwhile please review eclipse-platform/eclipse.platform.releng.aggregator#1740 and #1536

@elsazac elsazac marked this pull request as ready for review January 22, 2024 11:49
@elsazac
Copy link
Member Author

elsazac commented Jan 22, 2024

I have separated the bundles and this is ready for review. for some reason I am unable to squash the commits (error: could not detach HEAD). Any pointers?

@jukzi
Copy link
Contributor

jukzi commented Mar 8, 2024

I have separated the bundles and this is ready for review. for some reason I am unable to squash the commits (error: could not detach HEAD). Any pointers?

Please close this PR and create a separate PR for each bundle. I also second the change.

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.

None yet

7 participants