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.
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
Improve support for reading Delimited files #3424
Improve support for reading Delimited files #3424
Changes from 11 commits
6294ce7
c3113bb
6661caf
c039f62
70a7a6d
af2e1f9
6ca34a3
7bf1faf
9304a2d
e44e3fa
c4260d0
796dcb7
5a862a6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this file and Delimited_Reader be merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would avoid it -
Csv
is the old module that should be deprecated and removed onceDelimited
is on-par with its functionality.So making them separate makes it easier to see what is going to be removed. We keep it only because our Table tests heavily rely on this mechanism and
Delimited
is not yet fully-featured to replace it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough - perhaps a task to remove
Csv.enso
then so we don't forgetThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like this file should be in Internal as not for end user use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the
Delimited
type defined?read_file
feels the wrong name -read_delimited_file
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that you call it through the module name:
Delimited_Reader.read_file
so addingdelimited
is a bit redundant.Something like, if you have
StringParser
you usually doStringParser.parse
and notStringParser.parseString
(at least that's how I'd approach it, naming conventions can be a bit subjective I guess).I can rename to as you suggest :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
Delimited
type is of course defined inside of theFile_Format
, as it is part of this ADT. It can be technically moved, but if I moveDelimited_Reader
toInternal
, it shouldn't be there because it is a public part of the API. I put it inFile_Format
because all the other formats were kept there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, should be there just didn't see it being imported, so was confused that Delimited worked as opposed to
File_Format.Delimited
.Delimited_Reader.enso
feels the wrong name. I'd say justDelimited.enso
.The
read_file
inside it feels to general to me hence suggestingread_delimited
. Agree is repeating but this is an internal function and otherwise feels too close toFile.read
and also would follow theread_text
,read_bytes
pattern.