https://outerbounds.com/ logo
Join Slack
Powered by
Hello! What do you all think about <this issue>? O...
# dev-metaflow
r
Hello! What do you all think about this issue? Of course, it’s non-blocking but would be a HUGE quality of life improvement for us (and hopefully others), especially from a11y standpoint. If you all agree on the approach, happy to send a PR your way!
h
I don't mind getting this in, but since this is vendored code we will have to remember to apply the same patch if we ever update. Is this change already upstream?
r
nah the change is not upstream and i looked through some GH issues and it seems like Click refuses to apply this upstream, let me link them.
here’s a few: • https://github.com/pallets/click/issues/3022 • https://github.com/pallets/click/issues/1090 • https://github.com/pallets/click/issues/2282 • https://github.com/pallets/click/issues/2781 would a very detailed comment explaining the decision help? totally understand your pain around maintaining the vendored code though
h
maybe we can add a test so future folks remember to apply the patch?
r
sure, let me send a draft PR! appreciate a quick response
I put this together (very quickly), let me know if you have thoughts on it: https://github.com/Netflix/metaflow/pull/2798 -- Hopefully the CI will go through. I also did a little test manually to confirm that the colors do get rendered properly after the patch is applied
If u prefer, I can also attach screenshots/instructions from my manual test
hey very very gentle bump on this as we’d love to have it released and use it! totally happy to change the approach or anything you’d recommend
h
lgtm, just checking with OB folks
r
thank u!
h
merged, will go out in the next release
r
awesome, y’all are the best! getting changes upstream has never been easier 🫡
s
Thanks! Why not pass color to click.echo calls though?
At the moment, the vendoring process is quite straightforward and we have gone to great lengths to keep it that way. This will unfortunately open a Pandora's box and set a rather expensive precedent that will box our ability to upgrade packages
r
hey @square-wire-39606! happy monday!totally happy to revert my PR and go down that route. i get your concerns around not disrupting the vendoring process. if I am understanding it correctly, the main drawbacks for that approach would be: • we’d need to remember setting that arg for all 
click.echo(…)
calls in the future • if there are any third-party libraries that have committed to the 
FORCE_COLOR
standard, our current approach gets it for free, but with the 
click.echo(…)
approach, that won’t be the case. i am honestly less worried about this one. if you prefer passing 
color=True
to 
click.echo(…)
calls as the preferred approach, should we still introduce a 
FORCE_COLOR
env var and then do something like 
click.echo(color=bool(FORCE_COLOR)
[something on these lines] or just set it to true by default to keep it simple?
h
@square-wire-39606 i reverted the change: https://github.com/Netflix/metaflow/pull/2910
thankyou 1
r
@square-wire-39606 let me know once u get a chance to review my earlier message and I am happy to send out a PR with 
click.echo(…)
plumbing
s
Yes, we can keep it true by default so that the overall changes in the core are kept to a minimum
r
sounds good, i will work on an upstream change then
I have a draft here but I will look at it with fresh set of eyes tomorrow, add PR description, and put it up for review: https://github.com/Netflix/metaflow/pull/2921 --- But let me know if you notice anything out of ordinary in the meantime
5 Views
Open in Slack
PreviousNext
Powered by