search

cloudflare / pingora

A library for building fast, reliable and evolvable network services.
Apache License 2.0
20.21k stars 1.1k forks source link

Fixes #261 - Better `Default` implementation for `Opt` #272

Closed palant closed 1 day ago

palant commented 3 weeks ago

@johnhurt Sorry about grabbing this, Github doesn’t notify me about issues being assigned, so I only noticed when I already had everything done. Feel free to close this if you have a fix ready.

This replaces the Default implementation for Opt by an auto-generated one which happens to produce exactly the expected results (all boolean flags set to false and configuration file to None) and none of the side-effects listed in #261.

I checked and there is no piece of code in the repository (including tests and examples) actually depending on this Default implementation. So whatever the doc string is saying here, nothing was actually using it as an alias for Opt::parse. Given that, I considered not creating an alternative for calling Opt::parse without a clap::Parser dependency the best course of action.

palant commented 3 weeks ago

There seem to be lots of intermittently test failures. The test failures above were in code not even dependent on my changes. And locally I see pingora-load-balancing tests on main fail more than half the runs – anything between one and three failing tests.

Edit: After looking at pingora-load-balancing tests – is that connecting to actual servers on the internet? No wonder there are intermittent failures…

johnhurt commented 2 weeks ago

Don't worry about it. I should know by now that you only report issues you plan to fix 🤗. You're right that this is a better implementation of default. I added this originally for the quickstart guide so that it didn't require adding a dependency on (at the time) structopt. I'll accept this today and add a new function to Opt called parse that will accomplish the same thing without breaking convention.

palant commented 2 weeks ago

I should know by now that you only report issues you plan to fix 🤗.

Not really… I reported some issues that I simply don’t care about any more – my work-arounds got so elaborate that I would keep using them even if the issues were fixed.

palant commented 1 day ago

This landed: https://github.com/cloudflare/pingora/commit/86e6cd29121b8ae62e343ad73cd5cc7717b91126