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

Add Text.write Function #3518

Merged
merged 10 commits into from
Jun 13, 2022
Merged

Add Text.write Function #3518

merged 10 commits into from
Jun 13, 2022

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented Jun 9, 2022

Pull Request Description

Implements https://www.pivotaltracker.com/story/show/182309026

Important Notes

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide dist and ./run ide watch.

@radeusgd radeusgd marked this pull request as ready for review June 9, 2022 15:08
@radeusgd radeusgd requested a review from hubertp June 9, 2022 15:08
@radeusgd radeusgd force-pushed the wip/radeusgd/text-write-182309026 branch 2 times, most recently from e89edde to 7988555 Compare June 9, 2022 15:16
@@ -14,6 +14,7 @@ from Standard.Base.Data.Text.Encoding as Encoding_Module import Encoding, Encodi
from Standard.Base.Error.Problem_Behavior as Problem_Behavior_Module import Problem_Behavior, Report_Warning
import Standard.Base.Data.Locale
import Standard.Base.Meta
import Standard.Base.System.File.Existing_File_Behavior
Copy link
Member

@JaroslavTulach JaroslavTulach Jun 10, 2022

Choose a reason for hiding this comment

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

Importing something from Standard.Base.System.File seems to create a direct link from core strata of the language (as Text is one of the core types) towards much less important strata - e.g. I/O subsystem. I believe this goes against principles of modularity. I believe the language and core libraries shall be as small as possible, but modularly extensible - be it Java or Enso.

This decay of mutual dependencies between different stratas of StdLib is hard to prevent, when we don't have tools to ensure compile time failures when a core strata attempts to call higher level strata. However that doesn't mean we should encourage such dependencies. Having reference to File in Text.enso is an example of such strata crossing dependency and I'd suggest to avoid it.

I know where it leads (see The Entropy of Software page 60, Practical API Design Confessions of a Java Framework Architect book). I've been there at least twice. With NetBeans and with Java. We end up with (this time not object but) functionally oriented spaghetti code and it is going to be hard to fix the mutual dependencies in future.

I guess the primary motivation for having Text.write is usability. Our users are more likely to find the method on existing Text object than some static function in some other library. Such a desire goes against the modularization effort. Huge problem when designing Java APIs, but other modern languages have an answer to that: extension functions.

Doesn't Enso also supports extending other types with new functions/methods? Then the write function should be defined by File.enso, but register itself as an extension into Text type.

If possible, I'd find it way better design from modularity perspective.

Copy link
Collaborator

@hubertp hubertp Jun 10, 2022

Choose a reason for hiding this comment

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

I'm kind of inclined to agree that we are heading towards a spaghetti monster but at the same time PR implements what the ticket asked for

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm happy to move it to File if we decide that's the right approach.

Reg. extension functions - we have those yes, so I can rely on it. But I'm not sure if the language spec 1.0 includes them - IIRC they are removed so the situation may complicate a bit. Maybe we will be able to emulate the desired behaviour with a typeclass.

So I can move the definition to File.enso - question is - depending on the language spec, if we may need to move it back here. But since this is a small operation, I think it is ok to do so.

Copy link
Member

Choose a reason for hiding this comment

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

@JaroslavTulach That's a very good argument. Our stdlib is a little bit spaghetti because of lack of refactoring (which should be done in the meantime when implementing tasks) and also because of lack of proper tools to resolve it. It was one of the reasons why we are making changes to the language design. Currently, you don't have the tools in the language to resolve SOME of such a convoluted designs. In the past, we were thinking that we can introduce type classes to the language later in the future, but we realized that lack of them is very harmful right now - your comment shows one of such harmful behaviors The new lang design allows for making it nicer. We will fix the lang and then fix the libraries. On Monday the doc will be ready for reading, I'll ping you guys and I'd love to hear feedback about it from the perspective of this issue.

Copy link
Member

Choose a reason for hiding this comment

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

We've implemented it as an extension within File.enso. Once we have the design doc agreed we should do a sanity pass and make sure all is how we want it.

Copy link
Collaborator

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

I would probably favour File.wrtie_text text instead of Text.write file as well

@@ -89,6 +90,10 @@ public void delete() throws IOException {
truffleFile.delete();
}

public void move(EnsoFile target, CopyOption... options) throws IOException {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would probably benefit from #3471 which is the final stages of being merged

@radeusgd radeusgd force-pushed the wip/radeusgd/text-write-182309026 branch from 7988555 to 17cbe4d Compare June 10, 2022 11:06
@jdunkerley
Copy link
Member

We discussed File.write_text as part of the design process. It was felt more natural to flow to be working with the object we want to write to a file.

I think we may find we want to add File.write_text at some point later but this will be easier to judge once we have a more feature complete GUI.

@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Jun 10, 2022
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

Thank you. Defining Text.write in File.enso looks way nicer from an architecture perspective.

@mergify mergify bot merged commit a04825a into develop Jun 13, 2022
@mergify mergify bot deleted the wip/radeusgd/text-write-182309026 branch June 13, 2022 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants