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 support for different column separator in csv import logic #1096

Merged
merged 8 commits into from
Aug 16, 2024

Conversation

code-constructor
Copy link
Contributor

As we all know, CSV is not always "comma separated values". It is often separated by other characters such as ";" or "|". This PR opens the model layer to support multiple column separators. The implementation is now able to import files with "," and ";" separators. More may come.

Important to know: The normalized representation normalized_csv_str is still separated by comma (",").

I would also like to implement the view support. But I don't want to do anything without a little guidance on what would be a good solution. Maybe just a selector below the upload field or a hidden section with "advanced import options".

@zachgoll
Copy link
Collaborator

zachgoll commented Aug 16, 2024

@code-constructor I think this all looks good and I'm definitely supportive of adding it in since a lot of non-US imports will have ; as the separator.

In terms of the UI, what do you think of grabbing this information on the very first step?

CleanShot 2024-08-16 at 11 47 17@2x

Long term, we'll probably take a second pass at the design of this entire feature and will put this in a more permanent place, but for now this seems like it gets the job done.

@code-constructor
Copy link
Contributor Author

code-constructor commented Aug 16, 2024

Did an update of the Pull-Request. I've implemented your idea 👍. Thanks for the really fast response. Was a pleasure 🙏

Copy link
Collaborator

@zachgoll zachgoll left a comment

Choose a reason for hiding this comment

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

Awesome, very nice addition! Just added a little suggestion to fix some form spacing. Otherwise looks and works great.

app/views/imports/_form.html.erb Outdated Show resolved Hide resolved
Co-authored-by: Zach Gollwitzer <[email protected]>
Signed-off-by: Alexander Schrot <[email protected]>
@code-constructor
Copy link
Contributor Author

code-constructor commented Aug 16, 2024

Good catch & Committed 👍

@zachgoll zachgoll merged commit 4527482 into maybe-finance:main Aug 16, 2024
4 checks passed
@code-constructor code-constructor deleted the csv-col-sep-support branch August 16, 2024 19:46
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.

2 participants