Find By Atlas Identifier Sub Command #209
Conversation
|
Closing and reopening to rerun Travis. |
There was a problem hiding this comment.
Thanks, this looks great @Bentleysb!
Only request is to translate the binary atlas files for testing into text atlas files (atlas.saveAsText(WritableResource)) so those can be Diffed in the future. Also you can use @TestAtlas(loadFromTextResource = "blah.atlas.txt") to get direct access to the Atlas files!
There was a problem hiding this comment.
Great stuff @Bentleysb - minor nits and suggestions along the way.
| // If joining is requested and there are shards to join... | ||
| if (output.isPresent() && !this.shardNames.isEmpty()) | ||
| { | ||
| System.out.printf("Joining..."); |
There was a problem hiding this comment.
Nit: Maybe something more descriptive like "Stitching shards and saving to output {}"
| * Object type | ||
| * @return {@link Predicate} | ||
| */ | ||
| private <T extends AtlasObject> Predicate<T> idCheck() |
There was a problem hiding this comment.
Nit: id --> identifier to stay consistent with lack of abbreviations
| */ | ||
| private <T extends AtlasObject> Predicate<T> idCheck() | ||
| { | ||
| return object -> this.ids.contains(((Long) object.getIdentifier()).toString()); |
There was a problem hiding this comment.
Why do we need to cast as Long and do a toString()? Why not just see if the Set of type Long contains the object identifier?
| * {@link AtlasEntity} to create the string for | ||
| * @return formatted string | ||
| */ | ||
| private String formatAtlasObject(final Atlas atlas, final AtlasEntity entity) |
There was a problem hiding this comment.
We might be able to get away without passing in the Atlas, I believe every AtlasEntity should have a getAtlas() method, which will return the Atlas it belongs to.
| new AtlasReader(args).runWithoutQuitting(args); | ||
|
|
||
| // Reset System.out | ||
| System.setOut(originalOut); |
There was a problem hiding this comment.
Any chance something goes wrong on lines 29-31 and we never reset System.out? Possible solution could be a try/catch/finally block.
|
@matthieun, unfortunately I don't think text atlases can be used here. |
|
The issue here is we cannot have tests rely on the current binary representation of Atlas files. Once default Atlas binary relies on protobuf arrays for example, those tests would fail. I see the problem you have here, those Atlas files need to be binary in a folder. What I'd suggest is still store them as text in Github, but have your test create a temporary folder in which to save the binary version from text, then run your test, and finally do a cleanup. Does that sound ok? |
|
That makes sense and sounds like a plan. I will make that change. |
| } | ||
|
|
||
| @AfterClass | ||
| public static void deleteBianaryAtlases() |
| "The Atlas file to save the joined output to (optional). If not passed the found shards will not be joined and only appear in the console.", | ||
| String::toString, Command.Optionality.OPTIONAL); | ||
|
|
||
| private final Set<Long> ids = new HashSet<>(); |
There was a problem hiding this comment.
Nit: ids --> identifiers, to follow repo convention of no abbreviations.
MikeGost
dismissed
matthieun’s stale review
Requested changes (txt files) were made.
|
Owww, the binary Atlas files stayed in the PR :( Will have to remove them later. |
|
That was my mistake @matthieun - saw the txt files commit, but didn't check if the binary files were removed. Looks like @Bentleysb has followed up with a PR to remove them here: #222. Thanks! |
Description:
This adds an
AtlasFindByAtlasIdentifierSubCommand. The command has two required and one optional parameter. Required are an input directory of Atlas shards, and a list of Atlas identifiers that are to be found. Optional is a output file. This command will output information to the console about each Atlas identifier found. This information includes the item type, containing shard, and tags. Additionally, the shards that contain the matched Atlas identifiers will be joined and saved if an output file is specified.This command is based on the
AtlasFindByFeatureIdentifierLocatorSubCommand, but differs in 3 mayor ways. First, the new command takes Atlas identifiers, instead of feature identifiers. This means that you need not know an entities type to search for it. Second, the feature identifier search only uses the OSM part of the numeric identifier. The Atlas identifier search uses all of the identifier, allowing for specific edges to be searched for (as opposed to all edges with that OSM id). Finally, the new command has an option to merge the shards where matching entities were found and output them as a single atlas. This is very useful when analyzing large areas that have been sharded, and that otherwise would be un-viewable due to memory constraints.To accommodate the combining of the matched shards, the
AtlasJoinerSubCommandis updated to have an optional parameter where shard names can be specified. When present, the shard names are used as a filter whereby only those shards from the specified input directory are joined.Potential Impact:
Adds a useful command.
The
AtlasJoinerSubCommandupdate is fully backwards compatible.Unit Test Approach:
The
AtlasFindByAtlasIdentifierSubCommandhas its console output tested by capturing the output stream (using an new helper class) and searching for the expected Atlas shard names.The original function of
AtlasJoinerSubCommandand the new use in combination withAtlasFindByAtlasIdentifierSubCommandare tested by joining 3 and 2 tiny Atlas shards, respectively. The results are saved to temporary files and checked for any contents.Test Results:
Verification of search and joining actions were verified using known shard names and visual inspection of new Atlas files in JOSM. All test results were positive.
In doubt: Contributing Guidelines