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

CSV Writing #1894

Merged
merged 5 commits into from
Jul 22, 2021
Merged

CSV Writing #1894

merged 5 commits into from
Jul 22, 2021

Conversation

kustosz
Copy link
Contributor

@kustosz kustosz commented Jul 22, 2021

Pull Request Description

Writes CSVs.

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 documentation and configuration conforms to the markdown and YAML style guides.
  • All code has been tested where possible.

@kustosz kustosz added Type: Enhancement p-highest Should be completed ASAP labels Jul 22, 2021
@kustosz kustosz self-assigned this Jul 22, 2021
Copy link
Contributor

@iamrecursion iamrecursion left a comment

Choose a reason for hiding this comment

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

A few things I'd like to see addressed, but mostly looks great!

@@ -41,7 +41,7 @@ from Standard.Base.Data.Number.Extensions export all hiding Math, String, Double
from Standard.Base.Data.Noise export all hiding Noise
from Standard.Base.Data.Pair export Pair
from Standard.Base.Data.Range export Range
from Standard.Base.Data.Text.Extensions export Text, Split_Kind
from Standard.Base.Data.Text.Extensions export Text, Split_Kind, Line_Ending_Style
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want this exported by default rather than accessible as Text.Line_Ending_Style?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Line_Ending_Style seems rather clear to me on its own. And Text.Line_Ending_Style is a looot to type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I'd go with Text.Line_Ending. What if we ever introduce another kind of line ending style?

Comment on lines +721 to +722
example_to_csv = Examples.inventory_table.to_csv
to_csv : Boolean -> Boolean -> Text -> Line_Ending_Style.Line_Ending_Style -> Text
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
example_to_csv = Examples.inventory_table.to_csv
to_csv : Boolean -> Boolean -> Text -> Line_Ending_Style.Line_Ending_Style -> Text
example_to_csv = Examples.inventory_table.to_csv
to_csv : Boolean -> Boolean -> Text -> Line_Ending_Style.Line_Ending_Style -> Text

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't do this, ever :)

Comment on lines +731 to +732
- file: the file to write the generated CSV contents to. Note that other
files may be created or written to if `max_rows_per_file` is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the file exists and has contents? You should probably have a should_overwrite parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't do it for any other file-writing method though? So this seems consistent and if we want to change it, we should probably change all places where this could happen. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Then we probably should have it elsewhere, but not worth it for now. File an issue.

Comment on lines +757 to +758
example_to_csv = Examples.inventory_table.write_csv (Enso_Project.data / example_csv_output.csv)
write_csv : File.File -> Boolean -> Boolean -> Text -> Line_Ending_Style.Line_Ending_Style -> Nothing | Integer -> Nothing
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
example_to_csv = Examples.inventory_table.write_csv (Enso_Project.data / example_csv_output.csv)
write_csv : File.File -> Boolean -> Boolean -> Text -> Line_Ending_Style.Line_Ending_Style -> Nothing | Integer -> Nothing
example_to_csv = Examples.inventory_table.write_csv (Enso_Project.data / example_csv_output.csv)
write_csv : File.File -> Boolean -> Boolean -> Text -> Line_Ending_Style.Line_Ending_Style -> Nothing | Integer -> Nothing

@kustosz kustosz merged commit ca52757 into main Jul 22, 2021
@kustosz kustosz deleted the wip/mk/csv-write branch July 22, 2021 13:13
iamrecursion pushed a commit that referenced this pull request Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p-highest Should be completed ASAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants