Javadoc2 #72
Javadoc2 #72
Conversation
I don't think "Iterator" was really the right metaphor for what this class did. 1. Rename class and methods to match the new metaphor. 2. Javadoc for this class Update affected classes with method calls and the change in type.
Add promises to the programmer about how a CommitVisitor will be used.
- RepoDriller: Javadoc and move some logs around - Study: Javadoc - RepoDrillerException: Javadoc
There was a problem hiding this comment.
Thanks for the documentation! And thanks for the refactoring of the CommitVisitorIterator; RepoVisitor is way better!
This PR looks good to me.
I responded to some of your TODOs here. As soon as we decide what to do with them, I'll merge this one.
Also, I suppose the tests are passing, right? I'll configure travis to run the tests for PRs too, so that we see it on Github!
| * Close all of the PersistenceMechanism's associated with visitors. | ||
| */ | ||
| void closeAllPersistence() { | ||
| /* TODO: This API doesn't make sense to me. Why are the persistence mechanisms closed by this class? |
There was a problem hiding this comment.
Where would you close them? Would you leave for the client to close them?
The problem is that, then, the user can't write a beautiful fluent instantiation of the repo:
new RepositoryMining().
...
.process(visitor, new CSVFile(...))
The user would have to store new CSVFile() in a variable, and then, close it.
In addition, I'd not expect the program to do anything else after repodriller is done. So, I guess I implemented it as a 'courtesy' to the user.
What do you think?
There was a problem hiding this comment.
I agree that the inline PersistenceMechanism instantiation is lovely, and that if code is written in the functional style then RepositoryMining must close up the PersistenceMechanism's it's been given.
On the other hand, there's a mismatch between where the PM is created (client level) and where it is closed (framework level), which is surprising.
How about we overload RepositoryMining.process with a "close the PM for me" flag, and have the current 2-arg version always close the PM?
There was a problem hiding this comment.
Let's open an issue to discuss about that (#77), so that I can move on with this PR
| try { | ||
| log.info("-> Processing " + commit.getHash() + " with " + visitor.name()); | ||
| visitor.process(currentRepo, commit, writer); | ||
| } catch (CSVFileFormatException e) { |
There was a problem hiding this comment.
not sure why this exception is here... This deserves an issue: #74
There was a problem hiding this comment.
Perhaps we should define what the behavior is if a CommitVisitor throws an exception.
Options:
- We could remove the visitor from the RepoVisitor.
- We can mask the exception.
- We can throw.
There was a problem hiding this comment.
I believe the behavior is already correct there. If an exception happens, repodriller logs it, and moves forward. It could maybe notify the study or the visitors about the error.
But this particular CSVFileFormat, not sure what it does there. I think it halts the problem as soon as it notices that the output is not being saved. It makes sense to completely stop the execution. But we should make it more general, so that this behaviour is the same through all the persistence mechanisms! This should be in #74
| * @param inherit True for inherit, else false. | ||
| * @return this, for chaining | ||
| */ | ||
| public SimpleCommandExecutor inheritEnv(boolean inherit) { |
There was a problem hiding this comment.
do we have a need for this feature?
There was a problem hiding this comment.
The current implementation does this implicitly when no environment variables are specified (it returns null when the list is empty). I thought it would be nice to make that behavior explicit.
|
Ok, just noticed the 'check' next to the commits! Tests are passing!! 👍 |
|
Was just going to release 1.3.1 due to a bug fixed by @ishepard, and maven compiler started to complain about Javadoc. At first, I tried to fix the warnings, but then I gave up, and disabled the Javadoc strict mode (that comes with Java 8). We should decide whether we want it to be strict! |
|
Great, sorry for the issues. I'll double-check for that next time. |
Towards #70.
644d577 and edac36f clarify the API for a CommitVisitor:
RepoVisitor (formerly CommitVisitorIterator) now enforces this rule.