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

Deprecate --directory/-d flag in favor of os.Stat #259

Merged
merged 2 commits into from
Oct 13, 2023

Conversation

joanlopez
Copy link
Contributor

@joanlopez joanlopez commented Oct 9, 2023

One of the few things that @undef1nd and I found a bit weird when starting to explore the Grizzly command-line interface is the need to specify the --directory/-d flag to say that the <resource-path> is a directory, an even more confusing that it's mandatory for the grr pull command.

So, this is both:

  • A proposal to just get rid of this flag, and use os.Stat instead to determine whether the provided path <resource-path> is a directory or not
  • A direct question to @malcolmholmes: Am I missing something? Was there any other reason/need to use that flag?

Note that the suggestion here is to keep it, but as deprecated, just to avoid breaking changes straight away.

Thanks!

func Pull(resourcePath string, opts Opts) error {
log.Infof("Pulling resources from %s", resourcePath)
stat, err := os.Stat(resourcePath)
if err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably good idea to omit ErrNotExist errors here, cause in Pull operations, the local directory might not exist, and can just be created. Right? Wdyt? @malcolmholmes

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pull requires a dir, no? In which case, sure it can make the dir.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, agreed! I added a bunch of changes to handle that, as well as to test the expected behavior!

@malcolmholmes
Copy link
Collaborator

-d introduced a very different way of operating. Instead of parsing a single file, it processed a complete directory of files. This seemed like a big difference at the time justifying an argument to distinguish.

I'm more than happy though with your observation that the flag is superfluous. I guess I would keep -d, confirm that, in that case the arg is a dir, and allow the situation without an argument to also accept a dir.

@joanlopez
Copy link
Contributor Author

joanlopez commented Oct 10, 2023

I'm more than happy though with your observation that the flag is superfluous. I guess I would keep -d, confirm that, in that case the arg is a dir, and allow the situation without an argument to also accept a dir.

I think that'd be the behavior with the current set of changes:

  • It keeps the -d, just marked as deprecated (leaves a message when used).
  • It checks that the <resource-path> is a directory, when needed, despite of the flag (which means that if you use the flag it's also checked, as you suggested, I think).

Sounds good?

Copy link
Contributor

@undef1nd undef1nd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks, Joan, for making this improvement.

@joanlopez joanlopez merged commit 39a906d into master Oct 13, 2023
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