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 Import#json, Export#json, and Export#email #34

Merged
merged 4 commits into from
Jul 12, 2024

Conversation

benjaminwil
Copy link
Contributor

This pull request closes #26.

This pull request allows the importing and exporting of JSON files. Previously only CSVs were supported. The interfaces for importing and exporting JSON have been designed to work the same way they already work for the CSV interfaces.

I've added documentation to the README for all of the JSON interfaces, so I won't go into more detail here. Refer to the changes to the README.

Breaking changes

There is one breaking change here, which is that the Export#email_csv method has been renamed to Export#email. It takes a new :format argument which defaults to :csv. Users will need to update their application code accordingly:

-Export.email_csv(to: my_email)
+Export.email(to: my_email)

Copy link
Contributor

@stevepolitodesign stevepolitodesign 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 for taking the time to address this! I left an initial review, but I think this is close.

Also noting, I will need to update #33 based on this change.

.tool-versions Outdated Show resolved Hide resolved
lib/art_vandelay.rb Outdated Show resolved Hide resolved
lib/art_vandelay.rb Outdated Show resolved Hide resolved
@stevepolitodesign
Copy link
Contributor

Also, don't forget to update NEWS.md. Since this is the first unreleased feature, we can do something like this:

Unreleased

Add relevant info here.

*Your Name*

@stevepolitodesign
Copy link
Contributor

Noting, I also need to fix CI.

@benjaminwil
Copy link
Contributor Author

benjaminwil commented Jul 11, 2024

I've rebased all of my commits against main and updated them to:

  • resolve new linting errors reported by Standard on Ruby 3.3.4.
  • resolve all the discussion comments on this PR.

And I've added a new commit with an entry in the NEWS file. I followed the format of the existing entries but added my name to the end like you suggested in your comment. Let me know if I need to revise.

@benjaminwil
Copy link
Contributor Author

CI is now passing almost everywhere, except on Ruby 3.0. It fails on the lint.

3.0 is now passed end-of-life, anyway, so maybe the solution is to remove it from the CI version matrix 🔪

@benjaminwil
Copy link
Contributor Author

Alternatively we could skip linting for 3.0!

@stevepolitodesign
Copy link
Contributor

CI is now passing almost everywhere, except on Ruby 3.0. It fails on the lint.

3.0 is now passed end-of-life, anyway, so maybe the solution is to remove it from the CI version matrix 🔪

@benjaminwil now that #36 is merged you are free to rebase 👍

@benjaminwil
Copy link
Contributor Author

Rebased. 👍

Copy link
Contributor

@stevepolitodesign stevepolitodesign left a comment

Choose a reason for hiding this comment

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

This is great! I think there's an opportunity to further refactor this, but since this project is in its infancy, I'd rather get it out into the world.

Possible future improvements:

  • Creating Art::Vandelay::Import.{from_csv, from_json}
  • Creating Art::Vandelay::Export.{from_csv, from_json}
  • Setting the format as an instance variable to limit the need to pass it through multiple methods

@stevepolitodesign stevepolitodesign merged commit 468db73 into thoughtbot:main Jul 12, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wouldart_vandelay accept pull requests for a JSON adapter?
2 participants