#139: feature for making symlinks relative #140
Conversation
moritzLanger
assigned MattesMrzik and moritzLanger and unassigned moritzLanger and MattesMrzik
There was a problem hiding this comment.
Good Work 👍
I added some Minor suggestions for the main Code and one question on the If statement, but otherwise really well done
The Test should be a bit more separated for Clarity reasons in my opinion, as well as minor changes to be more similar to other Tests in the Project
cli/src/test/java/com/devonfw/tools/ide/io/FileAccessImplTest.java
Outdated
Show resolved
Hide resolved
cli/src/test/java/com/devonfw/tools/ide/io/FileAccessImplTest.java
Outdated
Show resolved
Hide resolved
cli/src/test/java/com/devonfw/tools/ide/io/FileAccessImplTest.java
Outdated
Show resolved
Hide resolved
cli/src/test/java/com/devonfw/tools/ide/io/FileAccessImplTest.java
Outdated
Show resolved
Hide resolved
…feature-for-making-symlinks-relative # Conflicts: # cli/src/main/java/com/devonfw/tools/ide/io/FileAccessImpl.java
There was a problem hiding this comment.
Nice implementation of the changed, and much clearer Test, Thank you.
A little Nice to have / Convention is to comment every Method annotated with @test with the following structure.
/**
* Test of {@link `Class`} `Method`,` optinal contition e. g. if fileacces = true`.
*/
|
You can also resolve all the Conversations started by me from the previous review |
There was a problem hiding this comment.
@MattesMrzik thanks for this PR and the great feature 👍
You have done a good job, though I still found some places that need some improvement.
Please have a look. I will immediately close the comments that just honor your fixes of typos and other things you found in existing code on your way but feel free to have a look as well.
cli/src/test/java/com/devonfw/tools/ide/io/FileAccessImplTest.java
Outdated
Show resolved
Hide resolved
cli/src/test/java/com/devonfw/tools/ide/io/FileAccessImplTest.java
Outdated
Show resolved
Hide resolved
cli/src/test/java/com/devonfw/tools/ide/io/FileAccessImplTest.java
Outdated
Show resolved
Hide resolved
cli/src/test/java/com/devonfw/tools/ide/version/VersionIdentifierTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
@MattesMrzik sorry for the extra round. This is almost perfect now but as I found something that looks like a bug but I was not sure, if I do not get it or if my suggested fix is 100% correct, I decided to give this back to you for a final completion. Thanks in advance 👍
cli/src/test/java/com/devonfw/tools/ide/io/FileAccessImplTest.java
Outdated
Show resolved
Hide resolved
hohwille
changed the title
…making-symlinks-relative' into feature/devonfw#139-feature-for-making-symlinks-relative
…evonfw#139-feature-for-making-symlinks-relative # Conflicts: # cli/src/main/java/com/devonfw/tools/ide/io/FileAccessImpl.java
There was a problem hiding this comment.
@MattesMrzik thanks for your nice rework. 👍
Ready for merge.
hohwille
added
the
story-review
hohwille
added
the
reviewed
closes #139
symlinkdefault behavior to work with relative instead of absolute paths. This can be toggled with a flag.symlink's behavior consistent between soft links and windows junctions.