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

Version and flags #182

Merged
merged 21 commits into from
Jun 17, 2024
Merged

Version and flags #182

merged 21 commits into from
Jun 17, 2024

Conversation

dustinblack
Copy link
Member

@dustinblack dustinblack commented May 8, 2024

Changes introduced with this PR

Some usability and help output improvements

  • Added shorthand input parameters
  • Removed duplication of help text in the code
  • Made the help output more Linux-y Used the flag package method for help output
  • Added a line return after displaying the version output
  • Allowed input to be empty
  • Read input file from context directory
  • Implement default configuration and allow for config param to be empty
  • Update readme

Note that the flag package accepts either - or -- ahead of all parameters, but in the help output I used more Linux-usual single - for shorthand param names and double -- for full param names.

The flag package also has a function to auto-generate the usage output, but I wasn't satisfied with the way it formatted it, so I stuck with the manual string definition.


By contributing to this repository, I agree to the contribution guidelines.

cmd/arcaflow/main.go Outdated Show resolved Hide resolved
@mfleader mfleader self-requested a review May 8, 2024 14:03
Copy link
Contributor

@jaredoconnell jaredoconnell left a comment

Choose a reason for hiding this comment

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

Looks good, but I have one comment.

cmd/arcaflow/main.go Outdated Show resolved Hide resolved
webbnh

This comment was marked as resolved.

@dustinblack
Copy link
Member Author

Maybe I'm a curmudgeon, but it's always bothered me that the command flags don't follow GNU conventions. If we stick with flag, I can concede the change for single-letter alternatives and just do the other small code cleanups.

An alternative is to use an argparse package that is modeled after the Python module, but we've had a pretty firm stance in the past about avoiding external dependencies, and that package isn't exactly widely adopted.

@dbutenhof
Copy link

Maybe I'm a curmudgeon, but it's always bothered me that the command flags don't follow GNU conventions. If we stick with flag, I can concede the change for single-letter alternatives and just do the other small code cleanups.

I don't have a strong religious affiliation with "GNU" conventions. Hey, I "grew up" with DCL, and then adopted to BSD UNIX way before GNU invented long option names. There are all sorts of variations, some packages allowing unique abbreviations, for example, and a wide divergent practice about whether to use both UNIX single-character options and GNU long options.

What bugs me here is that we're doing "short long options" ... with meaningless single character options using the GNU long option syntax which disallows combining options. (And Google's "we don't need no stinking double dash" arrogance doesn't help, either, but that's almost beside the point.)

An alternative is to use an argparse package that is modeled after the Python module, but we've had a pretty firm stance in the past about avoiding external dependencies, and that package isn't exactly widely adopted.

Yeah, I hear: but an external package that gives reasonable standard behavior instead of the ugly internal non-standard behavior? That, to me, is a harder call. It's almost a bug fix...

@webbnh

This comment was marked as resolved.

@dustinblack
Copy link
Member Author

dustinblack commented May 17, 2024

I've removed the single-character flags and conceded to using the flag package's built-in usage function. However, this work has revealed an existing problem with the inputs. The current help language implies that both the -input and the -config parameters should be optional, but in reality excluding either of these results in a failure. My golang skills aren't really good enough tackle this well, so I'd appreciate help with addressing this problem as part of this PR.

dbutenhof

This comment was marked as resolved.

Copy link

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

Having dropped the single character "shortcut" options, your PR description could use a fresh coat of paint 😆

webbnh

This comment was marked as outdated.

@mfleader
Copy link
Member

Maybe I'm a curmudgeon, but it's always bothered me that the command flags don't follow GNU conventions. If we stick with flag, I can concede the change for single-letter alternatives and just do the other small code cleanups.

An alternative is to use an argparse package that is modeled after the Python module, but we've had a pretty firm stance in the past about avoiding external dependencies, and that package isn't exactly widely adopted.

Yeah that's a golang feature, not a bug. Many (maybe most) default to this non-GNU convention, even though it's not required.

cmd/arcaflow/main.go Outdated Show resolved Hide resolved
@dustinblack
Copy link
Member Author

I've updated the help language a bit more, and I've added a default configuration so that the -config option is now truly optional.

I'm still not entirely satisfied. The -context parameter seems to have no practical function for the user. The help language implies that the workflow will be loaded from that directory, but in fact the paths for all of the inputs are relative to the current working directory.

dbutenhof

This comment was marked as resolved.

README.md Outdated Show resolved Hide resolved
cmd/arcaflow/main.go Show resolved Hide resolved
webbnh

This comment was marked as resolved.

@dustinblack dustinblack requested a review from webbnh June 13, 2024 20:11
webbnh

This comment was marked as resolved.

@dustinblack
Copy link
Member Author

dustinblack commented Jun 14, 2024

I took another look at the readme, considering that the engine is likely one of the primary entry points to the project and the one likely to garner the most stars and github-directed attention over time. With that in mind, I added an updated description of Arcaflow as a whole, a logo, a gif animation of running a workflow, and some additional links for more info. I also removed all references to the Python deployer except the last one because I think at this time it really only deserves footnote status because of its limited use case.

Copy link

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

The gif is cute. You waited long enough that the small files can actually be read before it moves on. Not sure how fundamentally useful this is, but it's a solid visual addition.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

Perfection! ❤️

Copy link
Contributor

@jaredoconnell jaredoconnell left a comment

Choose a reason for hiding this comment

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

Just one comment. Looks good.

README.md Show resolved Hide resolved
webbnh

This comment was marked as resolved.

Copy link

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

The new commit didn't remove my approval; but just for the heck of it, I'll re-approve.

@dustinblack dustinblack merged commit d1e42bc into main Jun 17, 2024
5 checks passed
@dustinblack dustinblack deleted the version-and-flags branch June 17, 2024 21:06
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.

5 participants