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

Fixes #261 - Better Default implementation for Opt #272

Closed
wants to merge 3 commits into from

Conversation

palant
Copy link
Contributor

@palant palant commented Jun 8, 2024

@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
Copy link
Contributor Author

palant commented Jun 8, 2024

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
Copy link
Contributor

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.

@johnhurt johnhurt added bug Something isn't working Accepted This change is accepted by us and merged to our internal repo labels Jun 14, 2024
@palant
Copy link
Contributor Author

palant commented Jun 14, 2024

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.

drcaramelsyrup pushed a commit that referenced this pull request Jun 28, 2024
---
Empty commit to trigger checks again
---
Another empty commit to trigger checks

Includes-commit: 0e9d853
Includes-commit: 1237b26
Includes-commit: 6fdaf38
Replicated-from: #272
drcaramelsyrup pushed a commit that referenced this pull request Jun 28, 2024
---
Empty commit to trigger checks again
---
Another empty commit to trigger checks

Includes-commit: 0e9d853
Includes-commit: 1237b26
Includes-commit: 6fdaf38
Replicated-from: #272
@palant
Copy link
Contributor Author

palant commented Jun 28, 2024

This landed: 86e6cd2

@palant palant closed this Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted This change is accepted by us and merged to our internal repo bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants