-
Notifications
You must be signed in to change notification settings - Fork 993
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
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @MichaelChirico and the rest of your teammates on Graphite |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
OK, moved the above into its own issue to keep focused comments on this PR: #6110. |
I also don't understand the codecov issue. No such failure locally with |
There was a problem hiding this 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
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 |
667cf47
to
74ce6de
Compare
Some more changes here, mainly to rename some variables/add comments for readability. |
please try to avoid force-push if possible |
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 / |
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: (I compared |
74ce6de
to
0ea2800
Compare
great, thanks |
Towards #6105. This PR is a simple refactor to reduce code nesting / improve readability. It makes the actual implementation of #6105, #6107, much simpler.