Newlines revamp (reading and writing) #7

cllns · 2022-01-20T17:32:06Z

$INPUT_RECORD_SEPARATOR is only defined after require "english" (meaning it's an alias of $/ that's readable in english). We were only requiring it, outside of specs, in the memory adapter. Meaning, anyone using the real FileSystem (not the memory adapter), would not get multi-line files outputted. Instead, the lines were joined together without any separating character.

require "dry/files"
files = Dry::Files.new
files.write("hello_world.txt", ["hello", "world"]);
files.read("hello_world.txt")
# => "helloworld"

Additionally, when you're in a shell (depending on your shell settings), you can see the file also doesn't have a trailing newline.

$ cat hello_world.txt
=> helloworld%

This PR fixes all that :) I integrated @radar's work from #4 and extended it to the file system. This PR does fix the extra newlines issue that @solnic had with the generators. spec/unit/hanami/cli/commands/monolith/generate/slice_spec.rb

This does make the library less powerful because now it always adds a trailing newline. I think this is desirable, but it could make for unexpected file changes. For example, adding a line in the middle of a file will also add a newline to the end of the file. I think this is OK but we could handle those cases more elegantly, just don't think it's worth the increased complexity.

(It also changes the internal API, with an error, if you try to write anything but a string. This enforces a separation of concerns, where adapters now don't have to worry about joining or newline characters. They just write strings, already joined as desired from the Dry::Files#write implementation)

One thing to note is that using $INPUT_RECORD_SEPARATOR does not give us \r\n on Windows, if that's the intention. Instead of using a global variable, I think I'd prefer to have a Dry::Files::NEWLINE = "\n" constant. It'd be nice to get rid of them entirely. (We could also had a check for windows and define it as \r\n in that case).

It was only required for the MemoryFileSystem, not the real FileSystem, so writing an array would concatenate all the lines together without any spaces or newlines.

Specifically, we don't want to *join* the lines, but rather append to each line. This lets us avoid having to manually add a newline to the end, and handle them all at once. Also, we chomp every line on read (and before write) so we don't duplicate any newlines

This is so when they're read out, they do not contain line breaks at the end of each line's string. Later on, when this content is passed to write, it is joined with , which will add newlines

This separates the concerns, so the adapters only write **String**s, they don't do any joining or coercion.

cllns · 2022-01-21T16:17:18Z

Rebased for a cleaner commit history and rewrote the text in the PR too

jodosha

@cllns Good work. 👍 Could you please let me know what do you think about my feedback? Thanks :)

jodosha · 2022-06-20T06:48:38Z

lib/dry/files.rb

-    def write(path, *content)
-      adapter.write(path, *content)
+    def write(path, lines)
+      joined_lines = Array(lines).flatten.map { |line| line.chomp.concat(newline) }.join


@cllns Isn't this creating too many intermediate arrays? Can we use destructive counterparts (e.g. #map! instead of #map?

jodosha · 2022-06-20T06:49:46Z

lib/dry/files.rb

-      adapter.write(path, *content)
+    def write(path, lines)
+      joined_lines = Array(lines).flatten.map { |line| line.chomp.concat(newline) }.join
+      joined_lines = "" if joined_lines == "\n" # Leave it empty


@cllns Can we extract "" as EMPTY_STRING.dup?
Also, why "\n" is hardcoded, given we have @newline?

I switched to \n to newline but I don't understand EMPTY_STRING.dup.

I can extract an EMPTY_STRING = "" in this file if you want (though I think "" signifies an empty string well enough). I know what .dup does, but I don't understand why it would be helpful here. Can you explain?

jodosha · 2022-06-20T06:54:42Z

lib/dry/files/file_system.rb

      #
      # @since 0.1.0
      # @api private
-      def write(path, *content)
+      def write(path, content)
+        raise CanOnlyWriteStringError unless content.is_a?(String)


@cllns Given this is an adapter (private API), we control the arguments, this exception seems to be a redundant internal assertion. Is this necessary?

Gotcha. Was being defensive but you're right, it's not worth a custom Error

jodosha · 2022-06-20T06:58:33Z

spec/integration/dry/files_spec.rb

@@ -11,6 +10,12 @@
    FileUtils.remove_entry_secure(root)
  end

+  describe "$INPUT_RECORD_SEPARATOR" do
+    it "is not nil" do
+      expect($INPUT_RECORD_SEPARATOR).not_to be_nil


Suggested change

expect($INPUT_RECORD_SEPARATOR).not_to be_nil

expected = if windows?

"\r\n"

else

"\n"

end

expect($INPUT_RECORD_SEPARATOR).to eq(expected)

Then we can try to run CI on windows as well, if GHA allows that.

Based on my research "$INPUT_RECORD_SEPARATOR does not give us \r\n on Windows" (from above). I don't have a Windows computer to confirm this but that is what I found.

Besides, we don't need to test Ruby. This is a regression test to ensure we don't remove require "english" from this library (which is what this PR sought to fix in the first place). We don't care about the value, just that it's non-nil. And Windows CI sounds like something we should avoid if we can :)

cllns · 2022-06-27T22:55:32Z

@jodosha Comments addressed

jodosha · 2022-10-24T10:34:28Z

@cllns Could you please rebase with main? Thanks. :)

Require 'english' throughout, to define $INPUT_RECORD_SEPARATOR

a7ff007

It was only required for the MemoryFileSystem, not the real FileSystem, so writing an array would concatenate all the lines together without any spaces or newlines.

cllns requested a review from solnic as a code owner January 20, 2022 17:32

cllns marked this pull request as draft January 21, 2022 13:19

cllns force-pushed the readline-chomp-redux branch from 4ae201d to 2710d51 Compare January 21, 2022 13:38

cllns and others added 4 commits January 21, 2022 10:31

Chomp lines for MemoryFileSystem#readline

b4e7106

This is so when they're read out, they do not contain line breaks at the end of each line's string. Later on, when this content is passed to write, it is joined with , which will add newlines

Change file system adapters to only write Strings

182c874

This separates the concerns, so the adapters only write **String**s, they don't do any joining or coercion.

Leave empty files empty (i.e. without trailing newline)

6a5f92d

cllns force-pushed the readline-chomp-redux branch from f3bd6fc to 6a5f92d Compare January 21, 2022 15:20

Fix style guide violations

f1f4389

cllns marked this pull request as ready for review January 21, 2022 15:25

cllns added 2 commits January 21, 2022 11:37

Extract CanOnlyWriteStringError and update API docs

d456998

Remove unnecessary chomp

8d4dbc3

jodosha reviewed Jun 20, 2022

View reviewed changes

jodosha mentioned this pull request Jun 20, 2022

Chomp lines for readline #4

Closed

cllns added 2 commits June 27, 2022 17:55

Use in-place #map! to save memory

bcbac68

Use newline instead of hard-coding

1c5053c

cllns force-pushed the readline-chomp-redux branch from 147b933 to f0f4cff Compare June 27, 2022 22:56

cllns added 3 commits October 11, 2022 12:44

Remove CanOnlyWriteStringError, due to it being private API

c8fa3d9

Fix typo

d4d36fc

Capitalize name of file

d873b27

cllns force-pushed the readline-chomp-redux branch from 794a552 to d873b27 Compare October 11, 2022 16:47

timriley force-pushed the main branch from 2617b7c to ab5343a Compare November 21, 2022 10:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Newlines revamp (reading and writing) #7

Newlines revamp (reading and writing) #7

cllns commented Jan 20, 2022 •

edited

Loading

cllns commented Jan 21, 2022

jodosha left a comment

jodosha Jun 20, 2022

jodosha Jun 20, 2022

cllns Jun 27, 2022

jodosha Jun 20, 2022

cllns Jun 27, 2022

jodosha Jun 20, 2022

cllns Jun 27, 2022

cllns commented Jun 27, 2022

jodosha commented Oct 24, 2022

-      expect($INPUT_RECORD_SEPARATOR).not_to be_nil
+      expected = if windows?
+        "\r\n"
+      else
+        "\n"
+      end
+      expect($INPUT_RECORD_SEPARATOR).to eq(expected)

Newlines revamp (reading and writing) #7

Are you sure you want to change the base?

Newlines revamp (reading and writing) #7

Conversation

cllns commented Jan 20, 2022 • edited Loading

cllns commented Jan 21, 2022

jodosha left a comment

Choose a reason for hiding this comment

jodosha Jun 20, 2022

Choose a reason for hiding this comment

jodosha Jun 20, 2022

Choose a reason for hiding this comment

cllns Jun 27, 2022

Choose a reason for hiding this comment

jodosha Jun 20, 2022

Choose a reason for hiding this comment

cllns Jun 27, 2022

Choose a reason for hiding this comment

jodosha Jun 20, 2022

Choose a reason for hiding this comment

cllns Jun 27, 2022

Choose a reason for hiding this comment

cllns commented Jun 27, 2022

jodosha commented Oct 24, 2022

cllns commented Jan 20, 2022 •

edited

Loading