feat(#1602): discover depends on place #2432

maxonfjvipon · 2023-08-23T11:03:43Z

More details here

PR-Codex overview

This PR focuses on hiding a static constant DELIMITER to avoid code duplication in multiple places.

Detailed summary

Updated the OnReplaced class to include OnDefault in the list of places where DELIMITER is used.
Added a new class OnDefault to handle default object names.
Updated the DiscoverMojo class to use the OnDefault class for object discovery.

The following files were skipped due to too many changes: eo-maven-plugin/src/main/java/org/eolang/maven/DiscoverMojo.java

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

eo-maven-plugin/src/test/java/org/eolang/maven/DiscoverMojoTest.java

eo-maven-plugin/src/main/java/org/eolang/maven/DiscoverMojo.java

…-place

maxonfjvipon · 2023-08-29T10:41:35Z

@volodya-lombrozo please have a look one more time

volodya-lombrozo · 2023-08-29T13:11:50Z

eo-maven-plugin/src/main/java/org/eolang/maven/name/OnDefault.java

+ *
+ * @since 0.31.0
+ */
+public final class OnDefault implements ObjectName {


@maxonfjvipon I believe this change should be done in a separate PR. Please take a look that issue:
#2420

@volodya-lombrozo I'm not sure in such particular case. OnDefault is just one more ObjectName without complex behaviour.

@maxonfjvipon From the issue description:

Apparently we have to create a class which will parse raw string into two parts value and
optional version.

We have the same split function among many classes (The same we can say about DELIMETER static field). Hence, I believe this issue exactly about this.

@volodya-lombrozo I suggest to extend the #2420 so it affected this class too

@volodya-lombrozo I believe adding OnDefault class is not about solving the problem #2420. It's just one more object name. Yes, with code duplication, that will be somehow resolved after solving #2420.

@maxonfjvipon Let's do it for now. But I would recommend to solve #2420 issue first and then return to that PR.

@volodya-lombrozo yes, let's leave this as it. Object versioning has higher priority than refactoring at the moment

eo-maven-plugin/src/test/java/org/eolang/maven/DiscoverMojoTest.java

eo-maven-plugin/src/main/java/org/eolang/maven/name/OnVersioned.java

eo-maven-plugin/src/main/java/org/eolang/maven/DiscoverMojo.java

maxonfjvipon · 2023-08-29T15:43:27Z

@volodya-lombrozo please have a look one more time

volodya-lombrozo · 2023-08-30T09:24:25Z

eo-maven-plugin/src/main/java/org/eolang/maven/name/OnDefault.java

+ *
+ * @since 0.31.0
+ */
+public final class OnDefault implements ObjectName {


@maxonfjvipon From the issue description:

Apparently we have to create a class which will parse raw string into two parts value and
optional version.

We have the same split function among many classes (The same we can say about DELIMETER static field). Hence, I believe this issue exactly about this.

volodya-lombrozo · 2023-08-30T09:27:41Z

eo-maven-plugin/src/main/java/org/eolang/maven/name/OnVersioned.java

@@ -42,15 +43,24 @@ public final class OnVersioned implements ObjectName {
    /**
     * Default hash.
     */
-    private final CommitHash hsh;
+    private final Unchecked<CommitHash> hsh;


@maxonfjvipon Why do we need this overcomplication? I didn't find any usage of the next constructor:

public OnVersioned(final ObjectName origin, final Scalar<CommitHash> hash) { this(new Unchecked<>(origin::toString), new Unchecked<>(hash)); }

And to be honest, I don't see the reason for it.

@volodya-lombrozo here. Scalar is needed because explicit new OnDefault(tojo).hash() may fail with IndexOutOfBounds exception if tojo does not contain delimiter |

@maxonfjvipon I see, thank you. Maybe it's better to let it fail as fast as possible? Otherwise it looks like you are trying to swallow the exception or use exceptions for flow control (both of the approaches don't lead to a good design).

@volodya-lombrozo It should not fail at this step at all. Pay a bit more attention to the code:

new OnSwap( tojo.contains(OnReplaced.DELIMITER), new OnVersioned( replaced, new OnDefault(tojo)::hash ), replaced )

If condition tojo.contains(OnReplaced.DELIMITER) is FALSE, the first object will be never evaluated, the second one will be - replaced. But we need to construct it here and it should now fail on the construction step

@volodya-lombrozo maybe we will add one more object name for such cases, but there's only one for now, so there's no need

eo-maven-plugin/src/main/java/org/eolang/maven/DiscoverMojo.java

volodya-lombrozo · 2023-08-30T09:37:55Z

eo-maven-plugin/src/main/java/org/eolang/maven/DiscoverMojo.java

+     * Otherwise, try to append a version from tojo if there's no one already
+     *
+     * @param name Object name with tag on not.
+     * @param tojo Current tojo.


@maxonfjvipon Is it a tojo? Maybe we can name it as a "default object name", or "default version"?

@volodya-lombrozo it's an identifier of tojo

@maxonfjvipon But what is the identifier of tojo in this context?

@volodya-lombrozo just a name of it as string

@maxonfjvipon You treat it as an object name with hash, or I'm wrong?

@volodya-lombrozo with or without. But if I wrap tojo.identifier() into OnDefault somewhere before this function, I'll not be able check if it contains hash or not. The check should happen in this function, that's why I pass tojo String not as ObjectName

eo-maven-plugin/src/main/java/org/eolang/maven/DiscoverMojo.java

maxonfjvipon · 2023-08-31T07:58:47Z

@volodya-lombrozo please have a look one more time

volodya-lombrozo · 2023-08-31T08:47:17Z

@Graur Could you take a look, please?

maxonfjvipon · 2023-08-31T09:52:04Z

@Graur please review this one since Volodya is not able now

Graur

@maxonfjvipon I believe it's good to merge

maxonfjvipon · 2023-09-01T10:44:12Z

@yegor256 please have a look

yegor256 · 2023-09-01T12:10:19Z

@rultor merge

rultor · 2023-09-01T12:13:29Z

@rultor merge

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

rultor · 2023-09-01T12:26:50Z

@rultor merge

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

maxonfjvipon added 6 commits August 22, 2023 16:27

feat(objectionary#1602): added version to DiscoverMojo

81d587f

feat(objectionary#1602): tojo id

a91c3ea

feat(objectionary#1602): test

39c2c2c

feat(objectionary#1602): comma

ad4fc7f

feat(objectionary#1602): checkstyle

4b24715

feat(objectionary#1602): comment

ace422d

maxonfjvipon requested review from volodya-lombrozo and removed request for volodya-lombrozo August 23, 2023 11:35

volodya-lombrozo suggested changes Aug 28, 2023

View reviewed changes

eo-maven-plugin/src/test/java/org/eolang/maven/DiscoverMojoTest.java Show resolved Hide resolved

eo-maven-plugin/src/main/java/org/eolang/maven/DiscoverMojo.java Outdated Show resolved Hide resolved

maxonfjvipon added 6 commits August 28, 2023 12:58

Merge branch 'master' into feat/objectionary#1602/discover-depends-on…

60d4498

…-place

feat(objectionary#1602): fixed discovering

aff4fa6

feat(objectionary#1602): minor fixes

2493053

feat(objectionary#1602): refactoring

449a1d7

feat(objectionary#1602): checkstyle

e08d5eb

feat(objectionary#1602): windows path

e18b782

maxonfjvipon requested a review from volodya-lombrozo August 29, 2023 10:41

volodya-lombrozo suggested changes Aug 29, 2023

View reviewed changes

maxonfjvipon added 5 commits August 29, 2023 17:25

feat(objectionary#1602): identifier

5451f12

feat(objectionary#1602): wiithout foorce

a36381b

feat(objectionary#1602): refactor

96d4a1b

feat(objectionary#1602): fake maven

6f3a456

feat(objectionary#1602): description

b648616

maxonfjvipon requested a review from volodya-lombrozo August 29, 2023 15:43

volodya-lombrozo suggested changes Aug 30, 2023

View reviewed changes

maxonfjvipon commented Aug 30, 2023

View reviewed changes

eo-maven-plugin/src/main/java/org/eolang/maven/DiscoverMojo.java Show resolved Hide resolved

feat(objectionary#1602): refactoring

a7cca03

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

maxonfjvipon requested review from Graur and removed request for volodya-lombrozo August 31, 2023 09:51

Graur approved these changes Aug 31, 2023

View reviewed changes

rultor merged commit fd7475b into objectionary:master Sep 1, 2023
10 checks passed

maxonfjvipon deleted the feat/#1602/discover-depends-on-place branch September 18, 2023 12:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(#1602): discover depends on place #2432

feat(#1602): discover depends on place #2432

maxonfjvipon commented Aug 23, 2023 •

edited by pr-codex bot

Loading

maxonfjvipon commented Aug 29, 2023

volodya-lombrozo Aug 29, 2023

maxonfjvipon Aug 29, 2023

volodya-lombrozo Aug 30, 2023

maxonfjvipon Aug 30, 2023

maxonfjvipon Aug 30, 2023

volodya-lombrozo Aug 30, 2023

maxonfjvipon Aug 31, 2023

maxonfjvipon commented Aug 29, 2023

volodya-lombrozo Aug 30, 2023

volodya-lombrozo Aug 30, 2023

maxonfjvipon Aug 30, 2023 •

edited

Loading

volodya-lombrozo Aug 30, 2023

maxonfjvipon Aug 30, 2023

maxonfjvipon Aug 30, 2023

volodya-lombrozo Aug 30, 2023

maxonfjvipon Aug 30, 2023

volodya-lombrozo Aug 30, 2023

maxonfjvipon Aug 30, 2023 •

edited

Loading

volodya-lombrozo Aug 30, 2023

maxonfjvipon Aug 30, 2023 •

edited

Loading

maxonfjvipon commented Aug 31, 2023

volodya-lombrozo commented Aug 31, 2023

maxonfjvipon commented Aug 31, 2023

Graur left a comment

maxonfjvipon commented Sep 1, 2023

yegor256 commented Sep 1, 2023

rultor commented Sep 1, 2023

rultor commented Sep 1, 2023

feat(#1602): discover depends on place #2432

feat(#1602): discover depends on place #2432

Conversation

maxonfjvipon commented Aug 23, 2023 • edited by pr-codex bot Loading

PR-Codex overview

Detailed summary

maxonfjvipon commented Aug 29, 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

Choose a reason for hiding this comment

maxonfjvipon commented Aug 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxonfjvipon Aug 30, 2023 • 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

maxonfjvipon Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxonfjvipon Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

maxonfjvipon commented Aug 31, 2023

volodya-lombrozo commented Aug 31, 2023

maxonfjvipon commented Aug 31, 2023

Graur left a comment

Choose a reason for hiding this comment

maxonfjvipon commented Sep 1, 2023

yegor256 commented Sep 1, 2023

rultor commented Sep 1, 2023

rultor commented Sep 1, 2023

maxonfjvipon commented Aug 23, 2023 •

edited by pr-codex bot

Loading

maxonfjvipon Aug 30, 2023 •

edited

Loading

maxonfjvipon Aug 30, 2023 •

edited

Loading

maxonfjvipon Aug 30, 2023 •

edited

Loading