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

chasquid-util: honor --config_dir argument as chasquid does #60

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aaribaud-fgs
Copy link

chasquid-util usage mentions --config_dir but the option was ignored. This changes fixes that.

@albertito albertito self-assigned this Jan 11, 2025
@albertito
Copy link
Owner

Hi! Thanks for sending this pull request.

chasquid-util usage mentions --config_dir but the option was ignored. This changes fixes that.

Maybe I'm missing something, but chasquid-util usage mentions --configdir (without _), and that does seem to work. It is used extensively in tests too.

This pull request made me realize there's an inconsistency between chasquid (which uses --config_dir) and chasquid-util (which uses --configdir). That is unfortunate, and maybe worth fixing, but as far as I can see the documentation has the correct references.

However, it seems to me this patch is unnecessary, since chasquid-util works as documented? Maybe that discrepancy is the source of confusion?

Or am I missing something?

Thanks again!

@aaribaud-fgs
Copy link
Author

aaribaud-fgs commented Jan 13, 2025

Well I noticed the option name difference and I think I tried --configdir and somehow it failed, which is what prompted me to do this PR; while I am not proficient with go, I compared chasquid-util's handling of configDir with that in chasquid (which worked), and "ported" that, not noticing the code that later handles --configdir.

My bad.

What do you prefer: should I rewrite this PR to turn --configdir into --config_dir for chasquid-util across the whole repo (and make -the use of --configdir cause a deprecation warning), or should I just cancel it?

@albertito
Copy link
Owner

Well I noticed the option name difference and I think I tried --configdir and somehow it failed, which is what prompted me to do this PR; while I am not proficient with go, I compared chasquid-util's handling of configDir with that in chasquid (which worked), and "ported" that, not noticing the code that later handles --configdir.

My bad.

No worries, no need to apologize!

The flag mismatch is confusing.

What do you prefer: should I rewrite this PR to turn --configdir into --config_dir for chasquid-util across the whole repo (and make -the use of --configdir cause a deprecation warning), or should I just cancel it?

I think making chasquid-util accept --config_dir and then document that -configdir is deprecated is a good next step. If you want to adjust this patch to do that, I'd be happy to review it!

Thank you!

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.

3 participants