Skip to content
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

#1077 limit possible class names #1138

Merged
merged 1 commit into from
Sep 7, 2022

Conversation

mximp
Copy link
Contributor

@mximp mximp commented Sep 2, 2022

#1077 Trim Java class names

@mximp mximp requested a review from yegor256 September 2, 2022 10:55
@mximp
Copy link
Contributor Author

mximp commented Sep 2, 2022

@yegor256 please review

@yegor256
Copy link
Member

yegor256 commented Sep 2, 2022

@OlesiaSub pls, review

@yegor256 yegor256 requested review from OlesiaSub and removed request for yegor256 September 2, 2022 12:34
@mximp
Copy link
Contributor Author

mximp commented Sep 5, 2022

@OlesiaSub please check

<xsl:variable name="pre" select="concat($p, eo:clean($c))"/>
<xsl:choose>
<xsl:when test="string-length($pre)&gt;250">
<xsl:value-of select="concat(substring($pre, 1, 25), $alt)"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mximp what about possible conflicts between two long class names? If you just cut them, you will have them conflicted for sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yegor256 that's why I pass $alt which is combination of line & position. Within a file it should be unique postfix.

@yegor256
Copy link
Member

yegor256 commented Sep 5, 2022

@mximp I think it's a weak solution: just cutting the name to fit into the maximum size.

<xsl:variable name="pre" select="concat($p, eo:clean($c))"/>
<xsl:choose>
<xsl:when test="string-length($pre)&gt;250">
<xsl:value-of select="concat(substring($pre, 1, 25), $alt)"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, maybe you could introduce another naming convention for long class names to avoid possible collisions? For instance, if class name is too long you could just hash it somehow or smth like that.

Copy link
Contributor Author

@mximp mximp Sep 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OlesiaSub @yegor256 Hashing would be ideal solution. I tried to find relevant functions but had no luck
Do you have an idea how to create it in XSL language?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mximp take a look https://stackoverflow.com/questions/62388458/create-an-md5-encryption-function-using-pure-xsl, maybe it will be helpful..
But actually, if hashing is impossible/too complicated in xsl, can't you just create some names using XSL and then hash the long ones using non-xsl methods? (I'm not sure how this translator to java works, so maybe this idea is useless or impossible to implement too :) )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OlesiaSub thanks for the hint.. That's exactly what I'm talking about: there is no simple and elegant solution to implement it. The suggestion by the link is to use integration with Java which sounds overly complex.
Actually combination of file name, line and position of an object will give unique short name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mximp ok got it. can I ask Yegor to merge it if the Codacy check doesn't pass though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OlesiaSub I believe we ignore Codacy checks.
@yegor256 do you agree with uniqueness by name+line+position?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mximp @OlesiaSub it's possible to marry Java and XSL. You can implement a Shift in Java, where you will inject MD5 hashes into the right places. See, how it works here: https://github.com/polystat/far/blob/master/src/main/java/org/polystat/far/FaR.java#L110-L134

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yegor256 I've just checked suggested example. This implies that we will need to make transformation after all Java code is created. And we will just do text replacement.
I still suggest initial solution with line/position. It guaranties uniqueness across the file and also preserves some meaning of the name. We also keep original object name in @XmirObject annotation.
What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mximp I don't mind at all, as long as names are unique

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yegor256 then please merge

@yegor256
Copy link
Member

yegor256 commented Sep 7, 2022

@rultor merge

@rultor
Copy link
Contributor

rultor commented Sep 7, 2022

@rultor merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit a03af19 into objectionary:master Sep 7, 2022
@rultor
Copy link
Contributor

rultor commented Sep 7, 2022

@rultor merge

@yegor256 Done! FYI, the full log is here (took me 5min)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants