Support relative paths for legacy tag importer#123
Conversation
| public LegacyTagImporterConfig() | ||
| { | ||
| this(emptyList()); | ||
| this(null, emptyList()); |
There was a problem hiding this comment.
Using "null" this way always makes my danger sense tingle. How about an "Optional".
Alternatively a extra method to see if the path is set so that you can avoid the null checks in the calling code?
There was a problem hiding this comment.
👍 Using Optional.
| import java.nio.file.Path; | ||
| import java.util.List; | ||
|
|
||
| public class LegacyTagImporterConfig |
There was a problem hiding this comment.
Add Java doc to all public classes and methods please.
| } | ||
|
|
||
| public LegacyTagImporterFactory(final List<PathConfig> pathConfigs) | ||
| public LegacyTagImporterFactory(final LegacyTagImporterConfig config) |
There was a problem hiding this comment.
Why support two types of constructors that are so similar in effect but use different paradigms?
| return findConfig(path).isPresent(); | ||
| } | ||
|
|
||
| private Optional<PathConfig> findConfig(final Path file) |
There was a problem hiding this comment.
You changed the parameter to "path" in the calling function but kept is as "file" here.
Suggest using "path" uniformly.
| final Path relativePath = makeRelative(file); | ||
| return this.config.get().getPathConfigs().stream() // | ||
| .filter(config -> config.matches(relativePath)) // | ||
| .findFirst(); |
There was a problem hiding this comment.
What happens if multiple match?
There was a problem hiding this comment.
Wouldn't a hash map be more efficient here than iterating all path configs with each find?
There was a problem hiding this comment.
- For multiple matches we take the first matching path.
- We can't use a map because the matching logic is based on Java's path matcher.
| private Path makeRelative(final Path path) | ||
| { | ||
| final Path basePath = this.config.get().getBasePath(); | ||
| if (basePath == null) |
There was a problem hiding this comment.
Please add method to check if path is set instead of null check.
| { | ||
| return path; | ||
| } | ||
| return basePath.relativize(path); |
There was a problem hiding this comment.
Does the match still work if the paths in the path config are provided absolute?
There was a problem hiding this comment.
No. I created #127 to check for absolute paths.
| } | ||
|
|
||
| private Importer createImporter(final List<PathConfig> pathConfigs, final Path path) | ||
| private LegacyTagImporterConfig config(final PathConfig... pathConfigs) |
There was a problem hiding this comment.
Please rename to "configure" to be in line with the Java naming standards.
Also to avoid mix-up of this method with variables of the same name.
|
|
||
| private LegacyTagImporterConfig config(final String basePath, final PathConfig... pathConfigs) | ||
| { | ||
| return new LegacyTagImporterConfig(basePath == null ? null : Paths.get(basePath), |
There was a problem hiding this comment.
The null values make this piece of code really hard to read. Especially without context.
Codecov Report
@@ Coverage Diff @@
## develop #123 +/- ##
=============================================
+ Coverage 92.6% 92.63% +0.03%
- Complexity 811 816 +5
=============================================
Files 81 81
Lines 2203 2214 +11
Branches 196 197 +1
=============================================
+ Hits 2040 2051 +11
Misses 121 121
Partials 42 42
Continue to review full report at Codecov.
|
kaklakariada
dismissed
redcatbear’s stale review
@redcatbear approved the changes on the phone
2 participants
This is required for gradle plugin configuration