feat: add --clear flag #39
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@yegor256, please assign a reviewer for this PR |
|
@volodya-lombrozo pls, take a look |
volodya-lombrozo
suggested changes
Feb 6, 2023
There was a problem hiding this comment.
@advasileva About the builder pattern:
Builder. Terrible concept, since it encourages us to create and use big, complex objects. If you need a builder, there is already something wrong in your code. Refactor it so any object is easy to create through its constructors.
It's not my words :) I get it from that link.
There is a way how we can refactor the code in order to make it more maintainable.
I suppose we can create 3 different classes: DefaultSpeco, EolangSpeco, ClearSpeco - the names could be different of course, but I believe you get an idea.
| OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
| SOFTWARE. | ||
| --> | ||
| <xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform" id="SG" version="2.0"> |
There was a problem hiding this comment.
@advasileva Could you provide tests for that change, please?
There was a problem hiding this comment.
@volodya-lombrozo I understand correctly that it is better to implement this through .yaml configs with xmir?
There was a problem hiding this comment.
@advasileva Apparently, yes.
advasileva
force-pushed
the
feature/clear
branch
from
3c9c967
to
2d78e11
Compare
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #39 +/- ##
=============================================
- Coverage 89.36% 73.52% -15.84%
- Complexity 8 11 +3
=============================================
Files 2 6 +4
Lines 47 68 +21
Branches 3 5 +2
=============================================
+ Hits 42 50 +8
- Misses 5 18 +13
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
advasileva
force-pushed
the
feature/clear
branch
from
2e0aa53
to
ebfa31e
Compare
|
@volodya-lombrozo I edited the PR (rebase, split into |
volodya-lombrozo
suggested changes
Mar 15, 2023
There was a problem hiding this comment.
@advasileva I noticed that the refactoring has simplified the logic and made the code clearer, in my opinion, but we can simplify it even more. Overall, it's a good set of changes. Thank you.
PS: It would be nice to divide that PR into 2:
- Add
--clear-xmiroption - Refactoring
However, I don't think we have to divide the current PR, it's only for the future.
|
|
||
| @Override | ||
| public void exec() throws IOException { | ||
| Files.createDirectories(this.output()); |
There was a problem hiding this comment.
@advasileva Here is code duplication between all 3 specs implementations
There was a problem hiding this comment.
@volodya-lombrozo Yes, it is, because this is the only way to link classes (that I know of). If we used class inheritance and redefined methods in the style of the Factory Method pattern. But because of the composition, we have to explicitly prescribe the method we use, otherwise origin will take its methods instead of our overrides.
For this reason, I don't like the code I wrote (It has become nightmarishly non-expandable).
| */ | ||
| final class Speco { | ||
|
|
||
| public interface Speco { |
There was a problem hiding this comment.
@advasileva I believe we have to leave only one method in that class:
XML transform(XML xml);All work with files have to be moved out of Speco. The methods format, train, input, output are required only in particular Speco implementations and by that reason we don't have to define them for all other implementations.
There was a problem hiding this comment.
@volodya-lombrozo This is an attempt to make Speco classes reusable in multiple dimensions. In addition to the previous answer: I do not know how to use the method of the origin class calling overridden methods in composing classes (e.g. use exec of DefaultSpeco with output of EolangSpeco).
| new Speco(this.input, this.output, this.eolang).exec(); | ||
| Speco speco = new DefaultSpeco(this.input, this.output); | ||
| if (this.eolang) { | ||
| speco = new EolangSpeco(speco); |
There was a problem hiding this comment.
@advasileva Maybe EolangSpeco it is not a Speco ? I mean the Speco actually is a transformation rules that gives us pure XMIR.
If we have to save eo, or read from eo - we can just add utility class that will perform that transformation from/to XMIR. What do you think?
There was a problem hiding this comment.
@volodya-lombrozo I don't think utility class is a good idea -- https://www.yegor256.com/2014/05/05/oop-alternative-to-utility-classes.html#utility-classes-are-evil :)
There was a problem hiding this comment.
@volodya-lombrozo btw, the current design allows for changes in multiple dimensions (input/output format (EolangSpeco) and flags (ClearXmirSpeco), which can be combined. That is, it may become a problem in the future when new *Speco is added
Closed
advasileva
force-pushed
the
feature/clear
branch
from
e839afb
to
e60bb55
Compare
|
@volodya-lombrozo I took your code from #93, it really solves the problem with overriding methods. I also fixed tests and Thanks a lot for the example with the code, so it's clearer what was meant. Solving this problem with a single |
volodya-lombrozo
suggested changes
Mar 17, 2023
|
|
||
| @Override | ||
| public void exec() throws IOException { | ||
| Files.createDirectories(this.output); |
There was a problem hiding this comment.
@advasileva Technically, you still have code duplication between XmirWalk and EoWalk. But let's move on. For now, I just suggest to add puzzle for further refactoring. What do you think?
There was a problem hiding this comment.
@volodya-lombrozo I don't mind, in which class is it better to add puzzle?
There was a problem hiding this comment.
@advasileva To any, it doesn't matter
There was a problem hiding this comment.
@volodya-lombrozo done
| /** | ||
| * Takes source codes on EO, converts to xmir and applies the AOI tool. | ||
| * Applies XSL-transformations to file. |
There was a problem hiding this comment.
@advasileva It looks like we have xml here, it's not a file.
There was a problem hiding this comment.
@volodya-lombrozo fixed
| import org.objectionary.aoi.launch.LauncherKt; | ||
|
|
||
| /** | ||
| * The interface encapsulating applying of specialization to EO. |
There was a problem hiding this comment.
@advasileva Looks like it's not an interface
There was a problem hiding this comment.
@volodya-lombrozo fixed
volodya-lombrozo
approved these changes
Mar 17, 2023
There was a problem hiding this comment.
@advasileva Looks good to me. Thanks.
@yegor256 Could you merge that changes, please?
|
@yegor256 try to merge it, please. Maybe problem with time are already fixed... |
|
@rultor merge |
@advasileva @yegor256 Oops, I failed. You can see the full log here (spent 2hr) |
|
@yegor256 sorry, could you try merging again?.. |
|
@rultor merge |
@advasileva @yegor256 Oops, I failed. You can see the full log here (spent 2hr) |
|
@yegor256 merge this one too, please |
|
@rultor merge |
@advasileva @yegor256 Oops, I failed. You can see the full log here (spent 3hr) |
|
@volodya-lombrozo Сould you see why this test freezes and as a result the whole build cancels in time? Locally, the program from this test is successfully executed in 7 minutes. |
|
@yegor256 try merge pls |
|
@rultor merge |
|
@advasileva the puzzle #109 is still not solved. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
According to #11
To add the flag, I needed to pass another (fourth) parameter to the constructor, but this code did not pass the linter. There was an idea to create a
Settingsclass forSpecoexecution flags, but it looked redundant. As a result, I implemented a Builder pattern that buildsSpecowith the right flags (yes, we recently studied it at the university :)