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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow Configurable Converters on CSV #8858

Merged
merged 5 commits into from Apr 1, 2022
Merged

Allow Configurable Converters on CSV #8858

merged 5 commits into from Apr 1, 2022

Conversation

MichaelCordingley
Copy link
Contributor

@MichaelCordingley MichaelCordingley commented Oct 29, 2021 •
edited

This is a 馃檵 feature or enhancement.

Summary

Lets the user specify which converters to pass to CSV, defaulting to nil if not present in config. This is specifically so that numeric values in a CSV file can be parsed as something other than strings.

@MichaelCordingley MichaelCordingley marked this pull request as draft October 29, 2021 15:33
@MichaelCordingley MichaelCordingley marked this pull request as ready for review October 29, 2021 15:36
@MichaelCordingley
Copy link
Contributor Author

The build failures are for test/test_filters.rb:690 and test/test_excerpt.rb:103, neither of which relate to this change. Is master broken right now?

@ashmaroli
Copy link
Member

ashmaroli commented Oct 29, 2021 •
edited

@MichaelCordingley The tests are failing because you forgot to add commas after the parameters.

@MichaelCordingley
Copy link
Contributor Author

Oh. Wow. Dumb mistake of the day. Hold on a sec.

@MichaelCordingley
Copy link
Contributor Author

@ashmaroli There we go. I also had to move a few things around to make the linter happy.

@MichaelCordingley
Copy link
Contributor Author

I'm going to make a few more edits to make this more extensible.

@MichaelCordingley
Copy link
Contributor Author

@ashmaroli I see some PRs that have been sitting for multiple years here and my own has sat for a month already. Do you know if this project is accepting PRs?

I've attempted to contribute to enough projects where the PRs sit unremarked for years until I finally just close them, only to then receive a message from the maintainers surprised that I closed it.

@ashmaroli
Copy link
Member

@MichaelCordingley Yes, this project is open to contributions.
Regarding whether a PR gets immediate attention depends on many things, like:

  • whether the change is valuable to the user-base in general and not to just niche cases.
  • a maintainer is able to alot time and energy to understand, review, and finalize a merge.

@MichaelCordingley
Copy link
Contributor Author

@ashmaroli Awesome. Thanks! And for this PR, I'm wide open to altering my approach. The big thing is just to be able to have numeric values from data files.

@MichaelCordingley
Copy link
Contributor Author

@ashmaroli How long does it typically take for a PR to receive consideration?

@ashmaroli
Copy link
Member

@MichaelCordingley Apologies for letting this go stale.

Okay, this PR definitely needs some test coverage.

The existing implementation and the proposed implementation appears to be going in very different directions (even though it may not be so in reality). Jekyll already has a concept of converters. For a maintainer / contributor stumbling across this implementation at a future date is going to end up confused.
Therefore, please include a comment above the private method(s) clarifying what :converters refers to here.

These methods are going to be called for every .csv / .tsv files in a site. Therefore, generating config and consecutively calling config.fetch for each file is not optimal, especially when the result is going to remain the same irrespective of the CSV / TSV file paths.

@MichaelCordingley
Copy link
Contributor Author

@ashmaroli Updated.

@ashmaroli
Copy link
Member

Thanks for making the changes.
There are some style issues you need to resolve, though. (Please check failing test).
I've additionally left a comment regarding the use of symbol keys..

@MichaelCordingley
Copy link
Contributor Author

All fixed up.

ashmaroli
ashmaroli reviewed Mar 18, 2022
View reviewed changes
test/test_data_reader.rb Outdated Show resolved Hide resolved
lib/jekyll/readers/data_reader.rb Outdated Show resolved Hide resolved
@ashmaroli ashmaroli requested review from mattr- and parkr March 18, 2022 16:28
parkr
parkr approved these changes Mar 19, 2022
View reviewed changes
Copy link
Member

@parkr parkr left a comment

Choose a reason for hiding this comment

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

Sounds good. Might be good to include some documentation as well.

@MichaelCordingley
Copy link
Contributor Author

MichaelCordingley commented Mar 21, 2022 •
edited

I don't mind documenting, once I know for sure it's going in and in what form.

@ashmaroli
Copy link
Member

once I know for sure it's going in and in what form..

@MichaelCordingley Please elaborate.

@MichaelCordingley
Copy link
Contributor Author

I don't want to write documentation and then not have the PR merged or write documentation only to have to tweak some details. It sounds like we're basically there.

@ashmaroli
Copy link
Member

I don't want to write documentation and then not have the PR merged or write documentation only to have to tweak some details.

This already has an approval from one of the maintainers.
Additionally, I'm on board with the implementation as well.
So yes, this will get merged.

Documentation is necessary for every enhancement or feature so end-users know how to use the new feature.

@MichaelCordingley
Copy link
Contributor Author

MichaelCordingley commented Mar 21, 2022 •
edited

Fantastic. Where do I write it? In this PR or against a separate place?

@MichaelCordingley
Copy link
Contributor Author

Looks like it should be somewhere in https://github.com/jekyll/jekyll/tree/master/docs/_docs, so I'll author against there unless I hear otherwise from you.

@ashmaroli
Copy link
Member

Where do I write it? In this PR

Yes, to the same branch. Information related to data files in Jekyll is principally accessed at https://jekyllrb.com/docs/datafiles/. Improve that page with info related to this PR at an appropriate location.

@MichaelCordingley
Copy link
Contributor Author

Documented.

@MichaelCordingley
Copy link
Contributor Author

Also rebased and force pushed to clean up the history.

parkr
parkr approved these changes Mar 22, 2022
View reviewed changes
Copy link
Member

@parkr parkr left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for adding docs. Two comments.

docs/_docs/datafiles.md Outdated Show resolved Hide resolved
docs/_docs/datafiles.md Show resolved Hide resolved
@MichaelCordingley
Allow Configurable Converters on CSV
0a5643f 
Lets the user specify which converters to pass to `CSV`, defaulting to `nil` if not present in config. This is specifically so that numeric values in a CSV file can be parsed as something other than strings.
ashmaroli
ashmaroli previously requested changes Mar 23, 2022
View reviewed changes
Copy link
Member

@ashmaroli ashmaroli left a comment

Choose a reason for hiding this comment

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

Requires some amendments to the documentation.
Plus, you need to fix our check-spelling action failures.

P.S. Don't bother squashing / rebasing. We squash commits during merge.

docs/_docs/datafiles.md Outdated Show resolved Hide resolved
docs/_docs/datafiles.md Outdated Show resolved Hide resolved
docs/_docs/datafiles.md Outdated Show resolved Hide resolved
MichaelCordingley and others added 3 commits March 23, 2022 09:45
@MichaelCordingley @ashmaroli
Update docs/_docs/datafiles.md
8275b61 
Co-authored-by: Ashwin Maroli <ashmaroli@users.noreply.github.com>
@MichaelCordingley @ashmaroli
Update docs/_docs/datafiles.md
2cb1ca6 
Co-authored-by: Ashwin Maroli <ashmaroli@users.noreply.github.com>
@MichaelCordingley
Add explicit list of options.
b8d9340
@ashmaroli ashmaroli dismissed their stale review March 23, 2022 14:06

Requested changes have been made.

@MichaelCordingley
Copy link
Contributor Author

MichaelCordingley commented Mar 23, 2022 •
edited

Plus, you need to fix our check-spelling action failures.

It looks like it's taking issue with the link to the Ruby docs.

Warning: docs/_docs/datafiles.md: line 157, columns 71-77, Warning - `libdoc` is not a recognized word. (unrecognized-spelling)
Warning: docs/_docs/datafiles.md: line 157, columns 58-64, Warning - `stdlib` is not a recognized word. (unrecognized-spelling)

@MichaelCordingley
Copy link
Contributor Author

I think that gets everything. 馃

@ashmaroli
Copy link
Member

Thanks @MichaelCordingley
@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 66e3379 into jekyll:master Apr 1, 2022
jekyllbot added a commit that referenced this pull request Apr 1, 2022
@jekyllbot
Update history to reflect merge of #8858 [ci skip]
e7ce73d
github-actions bot pushed a commit that referenced this pull request Apr 1, 2022
@jekyllbot
Deploy docs from 66e3379
9473de4 
MichaelCordingley: Allow Configurable Converters on CSV (#8858)

Merge pull request 8858
@captn3m0 captn3m0 mentioned this pull request Jun 1, 2022
Closed
@jekyll jekyll locked and limited conversation to collaborators Apr 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@parkr parkr parkr approved these changes

@ashmaroli ashmaroli ashmaroli approved these changes

@mattr- mattr- Awaiting requested review from mattr-

Assignees
No one assigned
Labels
enhancement frozen-due-to-age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@MichaelCordingley @ashmaroli @parkr @jekyllbot