#1788: Created a commandlet to simulate the behaviour of ln -s#1847
#1788: Created a commandlet to simulate the behaviour of ln -s#1847KarimALotfy wants to merge 35 commits into
Conversation
Added Unit Test for LnCommandlet Class
Coverage Report for CI Build 26288406984Warning No base build found for commit Coverage: 71.115%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
|
|
jakozian
left a comment
jakozian
left a comment
There was a problem hiding this comment.
Nice approach! But i requested changes because most things regarding the issue can be done with FileAccess/FileAccessImpl.
Usage of FileAccess instead of duplication
…c or hard link Changed variable name of source to target
KarimALotfy
added
commandlet
…optional - Added -f flag to control overriding of already existing links - Changed ln description in Help.properties
hohwille
left a comment
hohwille
left a comment
There was a problem hiding this comment.
@KarimALotfy thank you for all your updates and explanations. 👍
Now all looks good to me and is ready for merge.
@jakozian you requested changes. Can you approve as well to unblock the merge?
|
Unfortunately a failing test is now also blocking the merge: Is that caused by a side-effect from a change on |
…s://github.com/KarimALotfy/IDEasy into feature/1788-add-ln-commandlet-to-create-links
…t to symlinks, since existing symlinks or junctions will be overwritten anyway
|
@hohwille the build now is successful after removing the failing test. It was not failing on my local since it is enabled only on Linux and Mac , and was actually unnecessary, since the -f flag doesn't have an effect on the link method, to keep existing functionality |
hohwille
left a comment
hohwille
left a comment
There was a problem hiding this comment.
@KarimALotfy thanks for your improvements. 👍
Sorry, if I still was not clear enough but my vision is that we make the boolean parameters override and relative configurable via flags.
You did this but only partially and I would love to have it consistent.
| void createHardLink(Path source, Path link, boolean override) { | ||
| if (override && Files.exists(link, LinkOption.NOFOLLOW_LINKS)) { | ||
| delete(link); | ||
| } |
There was a problem hiding this comment.
I would still expect this to be done also for symbolic links.
If we have an override flag added to link(Path source, Path link, boolean relative, PathLinkType type, boolean override) and JavaDoc says {@code true} to override existing links, {@code false} otherwise. then this looks inconsistent to me. Will Files.createSymbolicLink be idempotent? I guess not and mklink is not for sure.
So IMHO the implementation is not doing what you specified in the JavaDoc.
There was a problem hiding this comment.
Thanks for the suggestions, implemented the suggested changes !
- Converted StringProperty of paths to PathProperty - Added a -r flag to identify relative paths - Added condition override for creating symlinks
…s://github.com/KarimALotfy/IDEasy into feature/1788-add-ln-commandlet-to-create-links
…/1788-add-ln-commandlet-to-create-links
- Remove override parameter and force flag from link method and ln commandlet - Modified deleteLinkIfExists to handle all types of links - Modified LnCommandletTest to handle all OS's in the same tests, instead of separating them by EnabledOnOs
…s://github.com/KarimALotfy/IDEasy into feature/1788-add-ln-commandlet-to-create-links
…/1788-add-ln-commandlet-to-create-links
5 participants
This PR fixes #1788
Implemented changes:
Added ide ln -s commandlet to create file links.
Checklist for this PR
Make sure everything is checked before merging this PR. For further info please also see
our DoD.
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#921: fixed setup.bat). If no issue ID exists, title only.In Progressand assigned to you or there is no issue (might happen for very small PRs)with
internal