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

Refactor colClasses handling for readability #6106

Merged
merged 4 commits into from
Jun 1, 2024
Merged

Conversation

MichaelChirico
Copy link
Member

@MichaelChirico MichaelChirico commented Apr 29, 2024

Towards #6105. This PR is a simple refactor to reduce code nesting / improve readability. It makes the actual implementation of #6105, #6107, much simpler.

Copy link
Member Author

MichaelChirico commented Apr 29, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @MichaelChirico and the rest of your teammates on Graphite Graphite

@MichaelChirico MichaelChirico marked this pull request as ready for review April 29, 2024 21:51
@MichaelChirico

This comment was marked as off-topic.

@Anirban166

This comment was marked as off-topic.

@Anirban166

This comment was marked as off-topic.

@MichaelChirico

This comment was marked as off-topic.

@MichaelChirico

This comment was marked as off-topic.

@MichaelChirico

This comment was marked as off-topic.

@MichaelChirico
Copy link
Member Author

OK, moved the above into its own issue to keep focused comments on this PR: #6110.

@MichaelChirico
Copy link
Member Author

I also don't understand the codecov issue. No such failure locally with covr::package_coverage().

Copy link
Member

@tdhock tdhock left a comment

Choose a reason for hiding this comment

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

I try to avoid continue in my personal code "for readability" but you are right it helps in this case.
I looked at the rest of the code base to make sure continue is used elsewhere, and it is used quite a lot

Copy link

github-actions bot commented May 1, 2024

Comparison Plot

Generated via commit c8febb9

Download link for the artifact containing the test results: ↓ atime-results.zip

Time taken to finish the standard R installation steps: 11 minutes and 45 seconds

Time taken to run atime::atime_pkg on the tests: 3 minutes and 19 seconds

@MichaelChirico MichaelChirico force-pushed the fread-date-idate branch 2 times, most recently from 667cf47 to 74ce6de Compare May 4, 2024 06:45
@MichaelChirico
Copy link
Member Author

Some more changes here, mainly to rename some variables/add comments for readability.

@tdhock
Copy link
Member

tdhock commented May 4, 2024

please try to avoid force-push if possible

@MichaelChirico
Copy link
Member Author

Thanks for the call-out, I especially regretted it here.

For context, this is what the Graphite tooling does -- I love the PR stacking but there are some other pain points. I'm still trying to learn all the nuances of a "mixed stack" -- my impression is their tooling is optimized for whole teams using the Graphite UI exclusively, but here we switch back & forth between Graphite+GitHub UIs / gt+git CLI commands.

@MichaelChirico
Copy link
Member Author

FWIW it looks like Graphite has its own history logged, so the diff that got erased on git b/c of the force-push can be seen here:

https://app.graphite.dev/github/pr/Rdatatable/data.table/6106/Refactor-colClasses-handling-for-readability

(I compared v6 vs v9)

@tdhock
Copy link
Member

tdhock commented Jun 1, 2024

great, thanks

@MichaelChirico MichaelChirico merged commit 6a4bd54 into master Jun 1, 2024
4 checks passed
@MichaelChirico MichaelChirico deleted the fread-date-idate branch June 1, 2024 06:28
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.

None yet

3 participants