eclipse.platform.ui icon indicating copy to clipboard operation
eclipse.platform.ui copied to clipboard

make path names UNIX-friendly

Open elsazac opened this issue 1 year ago • 19 comments

remove white spaces from path names

elsazac avatar Jan 03 '24 05:01 elsazac

Test Results

   930 files  +  310     930 suites  +310   50m 15s :stopwatch: + 24m 2s  7 474 tests ±    0   7 320 :white_check_mark:  -     1  153 :zzz: ±  0  1 :x: +1  23 571 runs  +7 857  23 059 :white_check_mark: +7 664  511 :zzz: +192  1 :x: +1 

For more details on these failures, see this check.

Results for commit 740e525a. ± Comparison against base commit ad1dfcd5.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Jan 03 '24 05:01 github-actions[bot]

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.

iloveeclipse avatar Jan 05 '24 13:01 iloveeclipse

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 avatar Jan 05 '24 13:01 elsazac

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

elsazac avatar Jan 05 '24 13:01 elsazac

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)...

laeubi avatar Jan 05 '24 13:01 laeubi

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...

merks avatar Jan 05 '24 14:01 merks

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.

akurtakov avatar Jan 05 '24 14:01 akurtakov

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.

elsazac avatar Jan 05 '24 14:01 elsazac

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...

laeubi avatar Jan 05 '24 14:01 laeubi

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

elsazac avatar Jan 22 '24 05:01 elsazac

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

@iloveeclipse @HeikoKlare @akurtakov

I believe no one objects.

merks avatar Jan 22 '24 07:01 merks

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

image

merks avatar Jan 22 '24 07:01 merks

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

akurtakov avatar Jan 22 '24 07:01 akurtakov

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.

iloveeclipse avatar Jan 22 '24 07:01 iloveeclipse

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).

HeikoKlare avatar Jan 22 '24 08:01 HeikoKlare

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".

merks avatar Jan 22 '24 08:01 merks

Thanks all for the suggestions. Keeping it draft until i rework on this. Meanwhile please review https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/pull/1740 and https://github.com/eclipse-platform/eclipse.platform.ui/pull/1536

elsazac avatar Jan 22 '24 09:01 elsazac

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?

elsazac avatar Jan 22 '24 11:01 elsazac

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.

jukzi avatar Mar 08 '24 13:03 jukzi

With so many conflicts, no actions and request to split the change per bundle I'm closing this one. Please open dedicated PRs per bundle.

akurtakov avatar Sep 24 '24 14:09 akurtakov

Thanks for pointing this out. I will work on to create dedicated PRs per bundle

elsazac avatar Sep 26 '24 08:09 elsazac