feat(#1602): default empty versions near to the root object #2501

maxonfjvipon · 2023-09-19T15:26:08Z

What's done:

added flag versioned to Syntax, ParsingTrain, XeListener and few more classes
depends on the flag XeListener adds empty ver attribute to ALL objects
this empty ver will be replaced with default hash in the next PR
depends on the flag ParsingTrain applies move-versions-down.xsl transformation
added move-versions-down.xsl transformation that moves ver attribute from the top object to the root (.org). When this sequence of objects will be traspiled to java (smth like Phi.Ф.attr("org").get().attr("eolang").get()...) it will check if there is a "ver" attribute and paste hash right after Phi.Ф so it would be Phi.Ф.attr("hash-here").get().attr("org").get()...

PR-Codex overview

This PR focuses on enabling object versioning in the code.

Detailed summary

Added a new boolean flag versioned to the Syntax class constructor.
Modified the Syntax class constructor to accept the versioned flag.
Added a new method oprop to the Objects interface.
Added a new XSL file for applying changes to XML after processing all shifts.
Updated the ParsingTrain class to use the new XSL file for versioning.

The following files were skipped due to too many changes: eo-parser/src/main/java/org/eolang/parser/ParsingTrain.java, eo-parser/src/main/java/org/eolang/parser/XeListener.java, eo-parser/src/main/resources/org/eolang/parser/move-versions-deeper.xsl

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

maxonfjvipon · 2023-09-20T07:37:12Z

@volodya-lombrozo please review this one

volodya-lombrozo

@maxonfjvipon I've tried to understand the reason behind that changes and I failed. Could you please attach that pr to a particular issue or add more human-readable message, please?

volodya-lombrozo

@maxonfjvipon Maybe I have some misunderstanding here, but there are my questions:

Why do you mix adding boolean versioned and move-versions-down.xsl in the same PR?
Which part of the issue will be solved by that PR?

volodya-lombrozo · 2023-09-20T12:59:51Z

eo-parser/src/main/java/org/eolang/parser/Objects.java

+     * @param condition Condition
+     * @param key Key
+     */
+    void oprop(boolean condition, String key);


@maxonfjvipon Could you please rename this method? It is hard to understand what you mean by this abbreviation. I don't think you meant this.

@volodya-lombrozo first "o" stands for "optional". Property that will be added depends on giving condition

@maxonfjvipon Why do we need such a method, if in case of condition==false we can just avoid calling this method?

@volodya-lombrozo this method is used 8 times in XeListener. We either call oprop 8 times or replace these calls with if (this.versioned) { this.objects.prop(key); } at 8 places

@maxonfjvipon I don't know how it happened that we have xprop method, but both of them xprop and oprop:

Hard readable.

Might produce some issues.

@volodya-lombrozo there's nothing about overloading

prop - just adds property, uses attr from Xemlby

xprop - changes property by given xpath, uses xattr from Xembly. It was added here: feat(#2399): new syntax ebnf #2474

oprop - addes property if given condition is TRUE. Actually is a temporary method. When object versioning is implemented it will be removed and all its occurrences will be replaced with prop

eo-parser/src/test/resources/org/eolang/parser/packs/move-versions-down.yaml

eo-parser/src/main/resources/org/eolang/parser/move-versions-down.xsl

maxonfjvipon · 2023-09-20T14:10:22Z

@volodya-lombrozo

Why do you mix adding boolean versioned and move-versions-down.xsl in the same PR?

It just happened that way. I agree that these changes may be in separated PRs

Which part of the issue will be solved by that PR?

It's a preparation step for part 3 of the ticket. Version is moved to the "org" object. So when this xmir will be transpiled to java, to-java.xsl will check if "ver" attribute exists there. If so - to-java.xsl will add extra attr("hash").get() java code

maxonfjvipon · 2023-09-20T14:42:15Z

@volodya-lombrozo please have a look one more time

volodya-lombrozo · 2023-09-20T16:25:48Z

eo-parser/src/main/java/org/eolang/parser/Objects.java

+     * @param condition Condition
+     * @param key Key
+     */
+    void oprop(boolean condition, String key);


@maxonfjvipon I don't know how it happened that we have xprop method, but both of them xprop and oprop:

Hard readable.

Might produce some issues.

eo-parser/src/main/resources/org/eolang/parser/move-versions-deeper.xsl

eo-maven-plugin/src/main/java/org/eolang/maven/optimization/OptSpy.java

maxonfjvipon · 2023-09-21T08:55:08Z

@volodya-lombrozo please have a look one more time

volodya-lombrozo

@maxonfjvipon Could you please link that pr with the corresponding issue (#2503 I guess?)

volodya-lombrozo · 2023-09-21T09:14:19Z

eo-parser/src/main/java/org/eolang/parser/XeListener.java

@@ -347,8 +360,10 @@ public void enterApplicable(final ProgramParser.ApplicableContext ctx) {
            if (ctx.STAR() != null) {
                base = "tuple";
                this.objects.prop("data", "tuple");
+                this.objects.oprop(this.versioned, "ver");


@maxonfjvipon If we add ver attribute for all objects without checking versioned, will it break something?

@volodya-lombrozo I guess so. For example in DiscoverMojo we have the next code:

private Collection<String> names(final XML xml, final String tojo) { return new SetOf<>( new Mapped<>( (String name) -> this.versioned(name, tojo).toString(), new Filtered<>( name -> !name.isEmpty(), xml.xpath( String.join( "", "//o[", "not(starts-with(@base,'.'))", " and @base != 'Q'", " and @base != '^'", " and @base != '$'", " and @base != '&'", " and not(@ref)", "]/string-join((@base,@ver),'", OnReplaced.DELIMITER, "')" ) ) ) ) ); }

It does not check withVersions flag and joins base and ver. If there will be many empty ver attributes, I believe there will be troubles. So versioned field in XeListener is safer

@maxonfjvipon Ok, I've checked it myself. I put versioned=true everywhere (including Syntax constructor), then made 3 changes in XMIR.xsd, add-probes.xsl and OnReplaced.java and some more tests:

7 files changed, 14 insertions(+), 6 deletions(-)

Then I run the tests and got the next result:

[INFO] eo ................................................. SUCCESS [ 4.958 s] [INFO] eo-parser .......................................... SUCCESS [ 12.196 s] [INFO] eo-maven-plugin .................................... SUCCESS [02:31 min] [INFO] eo-runtime ......................................... SUCCESS [02:11 min] [INFO] ------------------------------------------------------------------------ [INFO] BUILD SUCCESS [INFO] ------------------------------------------------------------------------ [INFO] Total time: 05:00 min [INFO] Finished at: 2023-09-21T15:11:22+03:00 [INFO] ------------------------------------------------------------------------

It means we can use ver for all objects and everything will work fine. What is the most important - we don't need all this changes related to versioned field in many classes, including the notorious oprop method.

@volodya-lombrozo so you suggest replace one set of changes, which:

is safer

is more controllable (because it's just a boolean variable)

lets us do not have extra attributes where they are not needed

with other set of changes, that works for now, but uncontrollable and will lead to the cases, where we will have eo code without any hint to the versions, but there will be some confusing emtpy "ver" attributes in xmir.
The boolean variable is less evil here, I believe

@volodya-lombrozo if you personally don't like oprop method, I can remove it and replace its occurrences with its content in XeListener

@maxonfjvipon

Which type of 'safety' are you referring to? I run the tests and showed that everything works fine.

Why do we need the control here? Moreover, as I already mentioned, the solution with boolean values creates even more problems (like that and OCP violation).

The current implementation of XeListener in that PR does exactly what you mentioned: it adds empty "ver" attributes.

eo-parser/src/main/java/org/eolang/parser/ParsingTrain.java

eo-parser/src/main/resources/org/eolang/parser/move-versions-deeper.xsl

maxonfjvipon · 2023-09-21T10:22:16Z

@volodya-lombrozo please have a look one more time

volodya-lombrozo · 2023-09-21T12:17:41Z

eo-parser/src/main/java/org/eolang/parser/XeListener.java

@@ -347,8 +360,10 @@ public void enterApplicable(final ProgramParser.ApplicableContext ctx) {
            if (ctx.STAR() != null) {
                base = "tuple";
                this.objects.prop("data", "tuple");
+                this.objects.oprop(this.versioned, "ver");


@maxonfjvipon Ok, I've checked it myself. I put versioned=true everywhere (including Syntax constructor), then made 3 changes in XMIR.xsd, add-probes.xsl and OnReplaced.java and some more tests:

7 files changed, 14 insertions(+), 6 deletions(-)

Then I run the tests and got the next result:

[INFO] eo ................................................. SUCCESS [ 4.958 s] [INFO] eo-parser .......................................... SUCCESS [ 12.196 s] [INFO] eo-maven-plugin .................................... SUCCESS [02:31 min] [INFO] eo-runtime ......................................... SUCCESS [02:11 min] [INFO] ------------------------------------------------------------------------ [INFO] BUILD SUCCESS [INFO] ------------------------------------------------------------------------ [INFO] Total time: 05:00 min [INFO] Finished at: 2023-09-21T15:11:22+03:00 [INFO] ------------------------------------------------------------------------

It means we can use ver for all objects and everything will work fine. What is the most important - we don't need all this changes related to versioned field in many classes, including the notorious oprop method.

volodya-lombrozo · 2023-09-21T12:39:40Z

eo-parser/src/main/resources/org/eolang/parser/move-versions-deeper.xsl

+  <!--
+  Here we move "ver" attribute from the top object deeper to pre-last one (before <o base="Q"/>)
+
+  1. The reason of transformation - move version to pre-last object ("org"). Then on "transpile"


@maxonfjvipon It seems that you are tying to apply this transformation in order to solve hypothetical algorithm in the future (that nobody knows, except you). What do you think if we start implement that algorithm first? Then, the first implementation of the algorithm will reveal the need for that transformation (or not). Moreover by doing this it will be more than clear "why we need that transformation".

@volodya-lombrozo I believe it would be harder and slower to implement, because we will have to manipulate what is not yet in any form, even if not quite right.
When you don't know for sure how this whole feature should work, you can't just come and say "we need this here, this there, and this out there". You need to start with something small, maybe not quite elegant, just to have something in front of you so you can work with it

@maxonfjvipon Exactly:

You need to start with something small, maybe not quite elegant, just to have something in front of you so you can work with it

But you don't start your work with something random. The first thing you usually do is answer the question, "what do I want to achieve?" and then draw a skeleton. After that, you start adding "meat" to that skeleton. When we have a skeleton and an endpoint, we don't ask questions like "why do we need that change?" because it's obvious.
Otherwise, you'll find yourself in the situation we're currently facing: no clear reason for the change and no understanding of the final goal we aim to achieve. The next day, any developer could easily remove those changes, and they'd be justified in doing so.

Suddenly, I didn't find a good article how to do so, but may be this blog post might explain the idea a bit:

Let’s keep all these unknowns in mind as we try to solve the problem on the highest level of abstraction. Of course, we start with a test

volodya-lombrozo · 2023-09-21T14:19:39Z

eo-parser/src/main/resources/org/eolang/parser/move-versions-deeper.xsl

+  <!--
+  Here we move "ver" attribute from the top object deeper to pre-last one (before <o base="Q"/>)
+
+  1. The reason of transformation - move version to pre-last object ("org"). Then on "transpile"


@maxonfjvipon Exactly:

You need to start with something small, maybe not quite elegant, just to have something in front of you so you can work with it

But you don't start your work with something random. The first thing you usually do is answer the question, "what do I want to achieve?" and then draw a skeleton. After that, you start adding "meat" to that skeleton. When we have a skeleton and an endpoint, we don't ask questions like "why do we need that change?" because it's obvious.
Otherwise, you'll find yourself in the situation we're currently facing: no clear reason for the change and no understanding of the final goal we aim to achieve. The next day, any developer could easily remove those changes, and they'd be justified in doing so.

Suddenly, I didn't find a good article how to do so, but may be this blog post might explain the idea a bit:

Let’s keep all these unknowns in mind as we try to solve the problem on the highest level of abstraction. Of course, we start with a test

volodya-lombrozo · 2023-09-21T14:35:19Z

eo-parser/src/main/java/org/eolang/parser/XeListener.java

@@ -347,8 +360,10 @@ public void enterApplicable(final ProgramParser.ApplicableContext ctx) {
            if (ctx.STAR() != null) {
                base = "tuple";
                this.objects.prop("data", "tuple");
+                this.objects.oprop(this.versioned, "ver");


@maxonfjvipon

Which type of 'safety' are you referring to? I run the tests and showed that everything works fine.

Why do we need the control here? Moreover, as I already mentioned, the solution with boolean values creates even more problems (like that and OCP violation).

The current implementation of XeListener in that PR does exactly what you mentioned: it adds empty "ver" attributes.

maxonfjvipon added 11 commits September 19, 2023 11:44

feat(objectionary#1602): default empty versions

ae8fb3c

feat(objectionary#1602): versions down

b670d1f

feat(objectionary#1602): versioned

801768b

feat(objectionary#1602): checkstyle

1e784c5

feat(objectionary#1602): order

2872a7d

feat(objectionary#1602): right train

e111584

feat(objectionary#1602): pack

b757583

feat(objectionary#1602): endless

eb3a109

feat(objectionary#1602): dots

7b0be79

feat(objectionary#1602): dots x2

bf24801

feat(objectionary#1602): disable test

eee1b9f

maxonfjvipon requested a review from volodya-lombrozo September 19, 2023 17:41

volodya-lombrozo suggested changes Sep 20, 2023

View reviewed changes

feat(objectionary#1602): refactor

48def2e

maxonfjvipon requested a review from volodya-lombrozo September 20, 2023 14:41

volodya-lombrozo suggested changes Sep 20, 2023

View reviewed changes

maxonfjvipon added 6 commits September 21, 2023 10:49

Merge branch 'master' into feat/objectionary#1602/default-versions

e82ea28

feat(objectionary#1602): versioned + comment

8917018

feat(objectionary#1602): todo

43d9115

feat(objectionary#1602): typo

f9173e0

feat(objectionary#1602): checkstyle

4e8317b

feat(objectionary#1602): comments

395032b

maxonfjvipon requested a review from volodya-lombrozo September 21, 2023 08:54

volodya-lombrozo suggested changes Sep 21, 2023

View reviewed changes

feat(objectionary#1602): docs

442d162

maxonfjvipon requested a review from volodya-lombrozo September 21, 2023 10:21

volodya-lombrozo suggested changes Sep 21, 2023

View reviewed changes

maxonfjvipon marked this pull request as draft September 25, 2023 18:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(#1602): default empty versions near to the root object #2501

feat(#1602): default empty versions near to the root object #2501

maxonfjvipon commented Sep 19, 2023 •

edited

maxonfjvipon commented Sep 20, 2023

volodya-lombrozo left a comment

volodya-lombrozo left a comment

volodya-lombrozo Sep 20, 2023

maxonfjvipon Sep 20, 2023

volodya-lombrozo Sep 20, 2023

maxonfjvipon Sep 20, 2023 •

edited

volodya-lombrozo Sep 20, 2023

maxonfjvipon Sep 21, 2023

maxonfjvipon commented Sep 20, 2023 •

edited

maxonfjvipon commented Sep 20, 2023

volodya-lombrozo Sep 20, 2023

maxonfjvipon commented Sep 21, 2023

volodya-lombrozo left a comment

volodya-lombrozo Sep 21, 2023

maxonfjvipon Sep 21, 2023 •

edited

volodya-lombrozo Sep 21, 2023

maxonfjvipon Sep 21, 2023 •

edited

maxonfjvipon Sep 21, 2023

volodya-lombrozo Sep 21, 2023

maxonfjvipon commented Sep 21, 2023

volodya-lombrozo Sep 21, 2023

volodya-lombrozo Sep 21, 2023

maxonfjvipon Sep 21, 2023

volodya-lombrozo Sep 21, 2023

volodya-lombrozo Sep 21, 2023

volodya-lombrozo Sep 21, 2023

feat(#1602): default empty versions near to the root object #2501

Are you sure you want to change the base?

feat(#1602): default empty versions near to the root object #2501

Conversation

maxonfjvipon commented Sep 19, 2023 • edited

PR-Codex overview

Detailed summary

maxonfjvipon commented Sep 20, 2023

volodya-lombrozo left a comment

Choose a reason for hiding this comment

volodya-lombrozo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxonfjvipon Sep 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxonfjvipon commented Sep 20, 2023 • edited

maxonfjvipon commented Sep 20, 2023

Choose a reason for hiding this comment

maxonfjvipon commented Sep 21, 2023

volodya-lombrozo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxonfjvipon Sep 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxonfjvipon Sep 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxonfjvipon commented Sep 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxonfjvipon commented Sep 19, 2023 •

edited

maxonfjvipon Sep 20, 2023 •

edited

maxonfjvipon commented Sep 20, 2023 •

edited

maxonfjvipon Sep 21, 2023 •

edited

maxonfjvipon Sep 21, 2023 •

edited