add ctor specialization #23

advasileva · 2023-01-14T22:31:01Z

Implementation of #22

Test case

Now the following test passes:

[] > first
  [] > boom
    QQ
    .io
    .stdout > @
      "First!\n"

[] > second
  [] > boom
    QQ
    .io
    .stdout > @
      "Second!\n"

[x] > foo
  x
  .boom > @

[] > app
  seq > @
    foo
      first
    foo
      second

transforms to

[] > first
  [] > boom
    QQ
    .io
    .stdout > @
      "First!\n"

[] > second
  [] > boom
    QQ
    .io
    .stdout > @
      "Second!\n"

[x] > foo
  x
  .boom > @

[] > app
  seq > @
    foo_spec_x_first
      first
    foo_spec_x_second
      second

[x] > foo_spec_x_first
  x
  .boom > @

[x] > foo_spec_x_second
  x
  .boom > @

and outputs

First!
Second!

Limitation: works correctly only for objects with one free attribute.

Dev solutions

xcop action was deleted because it docker build fails in the workflow and xcop are already runs in the maven build.
eoc old version 0.10.0 is used because the latest release 0.10.1 fails on windows (I can add an issue, but there are already failed GHA there).
Tests are restructured: divided by cases and added parameterization of tests.

advasileva · 2023-01-16T05:16:18Z

@yegor256 PR is ready for review

yegor256 · 2023-01-16T05:27:58Z

@volodya-lombrozo please, help me review this

yegor256 · 2023-01-16T08:02:59Z

@advasileva eoc 0.11.1 should work just fine

advasileva · 2023-01-16T10:18:05Z

@yegor256 updated eoc to the latest version, it works, thanks)

volodya-lombrozo

@advasileva Several suggestions:

Try to inline variables as much as possible. Article
The PR is quite large. I believe It would be great to send that changes at least in 2 separate PRs: (1) for 2-1-to-objects and (2) 2-2-calls-replacement.
You test both transformations at once in a single test, It would be great to have separate unit tests for each transformation. Unit tests actually help to understand the code behaviour in different cases and improve overall architecture.

volodya-lombrozo · 2023-01-16T09:33:44Z

Makefile

+	mvn clean install -Dmaven.test.skip -Pqulice
+	cp target/speco-1.0-SNAPSHOT-jar-with-dependencies.jar speco.jar
+
+lint: ##@Dev Rebuild app without linting and tests


@advasileva Is it allowed to have several targets with the same name lint?
https://stackoverflow.com/questions/43718595/two-targets-with-the-same-name-in-a-makefile

@volodya-lombrozo In this case, it is a "copy-paste" error

volodya-lombrozo · 2023-01-16T10:18:24Z

src/test/java/org/eolang/speco/MainTest.java

+    @ParameterizedTest
+    @ValueSource(strings = {"simple"})
+    public void convertsFromXmir(final String name, @TempDir final Path temp) throws IOException {
+        final Path output = this.runSpeco(this.xmirs.resolve(name), temp, false);


@advasileva You can inline that variable

volodya-lombrozo · 2023-01-16T10:19:58Z

src/test/java/org/eolang/speco/MainTest.java

-        for (final Path path : Files.newDirectoryStream(output)) {
+    @ParameterizedTest
+    @ValueSource(strings = {"booms", "pets"})
+    public void convertsFromEo(final String name, @TempDir final Path temp) throws IOException {


@advasileva Probably you can inline output and produced variables.
Please read this article: https://www.yegor256.com/2015/09/01/redundant-variables-are-evil.html

volodya-lombrozo · 2023-01-16T10:21:14Z

src/test/java/org/eolang/speco/MainTest.java

+     * @param second Directory
+     * @throws IOException Iff IO error
+     */
+    private void compare(final Path first, final Path second) throws IOException {


@advasileva You can inline variables here too

volodya-lombrozo · 2023-01-16T10:22:02Z

src/test/java/org/eolang/speco/MainTest.java

-        } else {
-            base = this.xmirs;
-        }
+    private Path runSpeco(final Path base, final Path temp, final boolean iseo) throws IOException {


@advasileva The method could be static.
Probably we can also inline some variables here.

volodya-lombrozo · 2023-01-16T10:22:23Z

src/test/java/org/eolang/speco/MainTest.java

+     * @param second Directory
+     * @throws IOException Iff IO error
+     */
+    private void compare(final Path first, final Path second) throws IOException {


@advasileva The method could be static

volodya-lombrozo · 2023-01-16T10:23:53Z

src/test/java/org/eolang/speco/MainTest.java

+    }
+
+    /**
+     * Compares two directpries.


@advasileva Typo

volodya-lombrozo · 2023-01-16T10:24:23Z

src/test/java/org/eolang/speco/MainTest.java

+     *
+     * @param first Directory
+     * @param second Directory
+     * @throws IOException Iff IO error


@advasileva Did you mean "If"?

@volodya-lombrozo no, this is an abbreviation of "if and only if"

volodya-lombrozo · 2023-01-16T10:29:40Z

src/test/java/org/eolang/speco/MainTest.java

+     * @param second Directory
+     * @throws IOException Iff IO error
+     */
+    private void compare(final Path first, final Path second) throws IOException {


@advasileva Please, give more meaningful names for that variables. It's hard to understand their purpose. You can also add some information to JavaDoc

advasileva · 2023-01-16T14:07:47Z

@volodya-lombrozo I didn't catch point 2, is it better for me now to create two (or three with a separate PR for tests) branches, transfer changes there, and then make edits? Or first make edits to this PR, and then create new PRs?

volodya-lombrozo · 2023-01-16T14:14:36Z

@volodya-lombrozo I didn't catch point 2, is it better for me now to create two (or three with a separate PR for tests) branches, transfer changes there, and then make edits? Or first make edits to this PR, and then create new PRs?

@advasileva You can leave as is - it's for the future. As for tests - too, you can just create "@todo Write separate unit test".

advasileva · 2023-01-16T14:26:47Z

@volodya-lombrozo okay, a good opportunity to try pdd)

At first I thought of splitting it into several parts when I was writing #22, but I didn't do that so that no one would have to review and edit a lot of intermediate code.

volodya-lombrozo

@advasileva Good. Few small comments

volodya-lombrozo · 2023-01-16T16:33:24Z

src/test/java/org/eolang/speco/MainTest.java

        final StringWriter writer = new StringWriter();
        IOUtils.copy(process.getInputStream(), writer);
        final String[] output = writer.toString().split("\\r?\\n");
-        final String[] result = Arrays.copyOfRange(output, 11, output.length - 1);
-        return Arrays.asList(result);
+        return Arrays.asList(Arrays.copyOfRange(output, 11, output.length - 1));


@advasileva Why exactly 11?

@advasileva (Optional) If you want, you can use cactoos classes instead - they are more convenient, as to me:

new Split(new TextOf(process.getInputStream(), "\\r?\\n");

@volodya-lombrozo 11 is a magic number that I counted myself. That's how many lines the eoc output takes up until the program output itself. As far as I know, this is the output only for eoc link and I can read the program output only for eoc --alone dataize app.

@advasileva It's better to add such description of magic numbers into JavaDoc.

volodya-lombrozo · 2023-01-16T16:36:48Z

src/main/java/org/eolang/speco/Speco.java

@@ -90,8 +90,7 @@ public void exec() throws IOException {
            source = this.input;
        }
        for (final Path path : Files.newDirectoryStream(source)) {
-            final XML before;
-            before = Speco.getParsedXml(new XMLDocument(Files.readString(path)));
+            final XML before = Speco.getParsedXml(new XMLDocument(Files.readString(path)));
            final String after;
            if (this.eolang) {
                after = new XMIR(


@advasileva You have code duplication here Speco.applyTrain(before).toString(), apparently you can move that upper

volodya-lombrozo · 2023-01-16T16:43:25Z

.github/workflows/xcop.yml

@@ -1,15 +0,0 @@
---
-name: xcop


@advasileva We fixed problem with xcop. g4s8/xcop-action#4

@volodya-lombrozo It's still redundant (because it is checked in the maven build), but let it be

volodya-lombrozo · 2023-01-16T16:45:26Z

src/test/java/org/eolang/speco/MainTest.java


 /**
 * Test to test the operation of the command line tool.
 *
 * @since 0.0.1
+ * @todo #22:90min add separate unit-test for each transformation,
+ * which would run step by step and check the intermediate results,


@advasileva Can we add single indent for @todo description?

* @todo #22:90min add separate unit-test for each transformation, * which would run step by step and check the intermediate results, * for example, use yml packs as in dejump.

this helps to highlight the text of the task

volodya-lombrozo

@advasileva Looks good to me

advasileva · 2023-01-17T08:10:49Z

@yegor256 can we try to merge it?

yegor256 · 2023-01-17T12:06:10Z

@rultor merge

rultor · 2023-01-17T12:09:22Z

@rultor merge

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

rultor · 2023-01-17T12:14:49Z

@rultor merge

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

advasileva added 6 commits January 15, 2023 01:28

feat: enable multiple tests

deef3e0

refactor: update tests structure

7518951

feat: add spec versions coping

8361b39

feat: add calls replacement

4f13483

fix: specify eoc version

28d5d20

fix: rm xcom action

c6d9c06

advasileva marked this pull request as ready for review January 15, 2023 15:39

yegor256 requested a review from volodya-lombrozo January 16, 2023 05:28

volodya-lombrozo suggested changes Jan 16, 2023

View reviewed changes

fix: make edits to pr

cb7b817

advasileva force-pushed the feature/ctor-spec branch from f5d2bfb to cb7b817 Compare January 16, 2023 15:47

fix: describe pdd todo

3ac401f

advasileva requested a review from volodya-lombrozo January 16, 2023 16:02

volodya-lombrozo suggested changes Jan 16, 2023

View reviewed changes

fix: minor updates

ad0b0ad

advasileva requested a review from volodya-lombrozo January 16, 2023 18:26

volodya-lombrozo approved these changes Jan 17, 2023

View reviewed changes

rultor merged commit 6d26670 into objectionary:master Jan 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ctor specialization #23

add ctor specialization #23

advasileva commented Jan 14, 2023 •

edited

Loading

advasileva commented Jan 16, 2023

yegor256 commented Jan 16, 2023

yegor256 commented Jan 16, 2023

advasileva commented Jan 16, 2023

volodya-lombrozo left a comment •

edited

Loading

volodya-lombrozo Jan 16, 2023

advasileva Jan 16, 2023

volodya-lombrozo Jan 16, 2023

volodya-lombrozo Jan 16, 2023

volodya-lombrozo Jan 16, 2023

volodya-lombrozo Jan 16, 2023

volodya-lombrozo Jan 16, 2023

volodya-lombrozo Jan 16, 2023

volodya-lombrozo Jan 16, 2023

advasileva Jan 16, 2023

volodya-lombrozo Jan 16, 2023

advasileva commented Jan 16, 2023

volodya-lombrozo commented Jan 16, 2023

advasileva commented Jan 16, 2023

volodya-lombrozo left a comment

volodya-lombrozo Jan 16, 2023

volodya-lombrozo Jan 16, 2023

advasileva Jan 16, 2023

volodya-lombrozo Jan 17, 2023

volodya-lombrozo Jan 16, 2023

volodya-lombrozo Jan 16, 2023

advasileva Jan 16, 2023

volodya-lombrozo Jan 16, 2023

volodya-lombrozo left a comment

advasileva commented Jan 17, 2023

yegor256 commented Jan 17, 2023

rultor commented Jan 17, 2023

rultor commented Jan 17, 2023

add ctor specialization #23

add ctor specialization #23

Conversation

advasileva commented Jan 14, 2023 • edited Loading

Implementation of #22

Test case

Dev solutions

advasileva commented Jan 16, 2023

yegor256 commented Jan 16, 2023

yegor256 commented Jan 16, 2023

advasileva commented Jan 16, 2023

volodya-lombrozo left a comment • edited Loading

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

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

advasileva commented Jan 16, 2023

volodya-lombrozo commented Jan 16, 2023

advasileva commented Jan 16, 2023

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

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

volodya-lombrozo left a comment

Choose a reason for hiding this comment

advasileva commented Jan 17, 2023

yegor256 commented Jan 17, 2023

rultor commented Jan 17, 2023

rultor commented Jan 17, 2023

advasileva commented Jan 14, 2023 •

edited

Loading

volodya-lombrozo left a comment •

edited

Loading