-
Notifications
You must be signed in to change notification settings - Fork 82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#397 Graph infrastructure #402
Conversation
This pull request #402 is assigned to @fanifieiev/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @paulodamaso/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be no monetary reward for this job |
@fanifieiev ping |
@fanifieiev ping...here too.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HDouss Please take a look at the comments
* @throws IOException If fails | ||
*/ | ||
public XmlGraph(final Skeleton skeleton) throws IOException { | ||
this.nds = XmlGraph.build(skeleton); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HDouss I don't think it is a good idea to execute code inside constructor.
https://www.yegor256.com/2015/05/07/ctors-must-be-code-free.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fanifieiev generally we can execute code if it is to initialize a final field. I think it would be better than to make the field non final and execute the code later. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HDouss You can use org.cactoos.Scalar for that case.
In Cactoos project we have a lot of such cases.
The main idea is to make a late execution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fanifieiev Thanks.
* Calculates ingoing and outgoing connected nodes. | ||
* @return List of nodes connected to this node. | ||
*/ | ||
List<Node> connected(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HDouss What about connections
naming?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fanifieiev Ok.
"//methods/method[@ctor='false' and @abstract='false']" | ||
); | ||
final List<Node> result = new ArrayList<>(methods.size()); | ||
for (final XML method:methods) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fanifieiev absolutely, did not about this class. Thanks!
@fanifieiev Thanks for your review. Fixed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HDouss One more comment
* @throws IOException If fails | ||
*/ | ||
public XmlGraph(final Skeleton skeleton) throws IOException { | ||
this.nds = new Unchecked<>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HDouss Please use org.cactoos.scalar.Sticky
for caching the evaluated list:
this.nds = new Unchecked<>(
new Sticky<>(
() -> XmlGraph.build(skeleton);
)
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HDouss Thanks
@paulodamaso We are good to merge this PR
@paulodamaso ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HDouss Just one comment, please take a look
final String name = "name"; | ||
final Node.Simple node = new Node.Simple(name); | ||
new Assertion<>( | ||
"Must be more the 2 files", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HDouss I don't think that this message is related to what is being asserted, please fix it
@paulodamaso Thanks. Fixed. |
@paulodamaso ping |
@HDouss Thank you |
@rultor merge |
@paulodamaso OK, I'll try to merge now. You can check the progress of the merge here |
@HDouss @paulodamaso Oops, I failed. You can see the full log here (spent 5min)
|
@fanifieiev/z this job was assigned to you 5days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please. |
Job audit: performer @fanifieiev/z/z didn't make CR comments |
Resolving issue #397
Starting graph construction infrastructure.