-
Notifications
You must be signed in to change notification settings - Fork 140
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
Feat/#2764/add hash in xml #2781
Conversation
@volodya-lombrozo check please |
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.
@Yanich96 Could you, please:
- Link the current PR description with the issue you are trying to solve (https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
- Close the previous similar PR (fix(#2764): caching logic changed, added StHash, StHashTest, fix tests #2771) if you aren't going to continue with it.
* | ||
* @since 0.35.0 | ||
*/ | ||
public final class StHash extends StEnvelope { |
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.
@Yanich96 I have some doubts about the StHash
implementation and if we need it. I believe you can set the hash
function directly in XeEoListener
during the parsing. 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.
@volodya-lombrozo Hash code computes from node 'objects'. There isn't this node in parsing stage.
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.
@volodya-lombrozo Xembly directives will have to be run 2 times: 1 - to calculate the hash, 2 - to completely parse, and during the parsing process there may be XSD schema errors and parsing errors, as a result of which the hash calculation ceases to make sense.
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.
@volodya-lombrozo Hash code computes from node 'objects'. There isn't this node in parsing stage.
@Yanich96 Would exitProgram
help us here? It looks like the final parsing stage.
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.
@volodya-lombrozo Xembly directives will have to be run 2 times: 1 - to calculate the hash, 2 - to completely parse, and during the parsing process there may be XSD schema errors and parsing errors, as a result of which the hash calculation ceases to make sense.
We anyway will need to read XML twice
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.
@volodya-lombrozo I will write todo for it.
@@ -113,6 +113,7 @@ SOFTWARE. | |||
<xs:attribute name="version" type="xs:string" use="required"/> | |||
<xs:attribute name="revision" type="xs:string" use="required"/> | |||
<xs:attribute name="dob" type="xs:dateTime" use="required"/> | |||
<xs:attribute name="hash" type="xs:string" use="required"/> |
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.
@Yanich96 Returning to the discussion. Maybe we don't need the hash
attribute? Especially if we will save hash
as a file attribute.
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.
@volodya-lombrozo
I don't understand how to save hash as a file attribute.
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.
I've found this solution: https://stackoverflow.com/questions/18955475/add-custom-attribute-or-metadata-to-file-java
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.
@volodya-lombrozo In OptCahed.java the method 'contains(final XML xml)' get XML, but not file. We should get hash code of program, because we can't have file attributes. This reason is saving hash code in XML.
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.
@Yanich96 You can always calculate the hash on the fly, like hash(xml);
and compare it with the cache file attribute.
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.
@volodya-lombrozo I have done todo. This line will be deleted after it.
@@ -47,6 +47,7 @@ | |||
* | |||
* @since 0.1 | |||
*/ | |||
@SuppressWarnings("PMD.TooManyMethods") |
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.
@Yanich96 I believe, you don't need that line. Could you please, remove the line and run the following command:
mvn qulice:check -Pqulice
I did it myself and the linter said nothing about methods number.
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.
@volodya-lombrozo Test failed : PMD: eo-parser/src/test/java/org/eolang/parser/EoSyntaxTest.java[50-250]: This class has too many methods, consider refactoring it. (TooManyMethods)
I used this command.
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.
@Yanich96 Ok. Let's leave it as is.
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.
@volodya-lombrozo This line will be deleted in the next pr, when I will transfer hash code in file attributes.
* @return String hash of this XML. | ||
* @throws NoSuchAlgorithmException If fails. | ||
*/ | ||
public String compute() throws NoSuchAlgorithmException { |
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.
@Yanich96 This method contains lots of redundant variables. https://www.yegor256.com/2015/09/01/redundant-variables-are-evil.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.
@Yanich96 The method name looks a bit misleading. The method returns a value, but you use verb here. Maybe it's better to use a noun?
@volodya-lombrozo check please |
@volodya-lombrozo I have fixed redundant variables in method StHash.compute(). Other comments will be resolved in added issues. |
* @return String hash of this XML. | ||
* @throws NoSuchAlgorithmException If fails. | ||
*/ | ||
public String compute() throws NoSuchAlgorithmException { |
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.
@Yanich96 The method name looks a bit misleading. The method returns a value, but you use verb here. Maybe it's better to use a noun?
* @throws NoSuchAlgorithmException If fails. | ||
*/ | ||
public String compute() throws NoSuchAlgorithmException { | ||
final BigInteger number = new BigInteger( |
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.
@Yanich96 number
is a redundant variable.
this.xml.nodes( | ||
"/program/objects" | ||
) | ||
.toString().getBytes() |
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.
@Yanich96 Here you apply toString
method to a List
instance, which is "ok" in this case (since the list contains strings.) However, why don't you use just xml.toString
? It looks simpler and doesn't require additional computations like applying paths
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.
@volodya-lombrozo I use '/program/objects' for creating the hash code. If 'xml.toString' is used, hash code will be created from all xml.
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.
@volodya-lombrozo Using the entire xml will cause the hash code to be inconsistent.
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.
@Yanich96 Why?
.toString().getBytes() | ||
) | ||
); | ||
final StringBuilder hash = new StringBuilder(number.toString(16)); |
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.
@Yanich96 Could you just use Hex.encodeHexString()
from Apache?
https://stackoverflow.com/a/9655275/10423604
Seems, we have this library in the classpath.
Closes #2764
What was done:
PR-Codex overview
This PR focuses on adding a new 'hash' attribute to the 'program' node in XML files.
Detailed summary