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

Add metaflow-diff to main CLI #2238

Closed
wants to merge 7 commits into from
Closed

Conversation

npow
Copy link
Contributor

@npow npow commented Jan 31, 2025

Had to shorten the help text to prevent it being chopped off:

metaflow help

Usage: metaflow [OPTIONS] COMMAND [ARGS]...

Options:
  --help  Show this message and exit.

Commands:
  admin        Execute admin commands.
  code         Metaflow code commands
  debug        Post/Pre Step debugging commands
  develop      Metaflow develop commands
  environment  Execution environment related commands.
  help         Show all available commands.
  status       Show flows accessible from the current working tree.
  tutorials    Browse and access the metaflow tutorial episodes.
  workbench    Workbench related commands
metaflow code --help

Usage: metaflow code [OPTIONS] COMMAND [ARGS]...

  Metaflow code commands

Options:
  --help  Show this message and exit.

Commands:
  diff       Do a 'git diff' of the current directory and a Metaflow run.
  diff-runs  Do a 'git diff' between two Metaflow runs.
  patch      Create a patch file for the current dir with a Metaflow run.
  pull       Pull the code of a Metaflow run.

@npow npow requested a review from tuulos January 31, 2025 06:48
diropt = f" --directory={path.rstrip('/')}"
else:
diropt = ""
echo("Apply the patch by running:")
Copy link
Contributor

Choose a reason for hiding this comment

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

echo and echo_always are a bit confusing here (one calls the other with some foreground, etc). Also, not sure if we want to respect the quiet flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what is your suggestion? get rid of echo?

Copy link
Contributor

@romain-intel romain-intel left a comment

Choose a reason for hiding this comment

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

Some comments.

diropt = f" --directory={path.rstrip('/')}"
else:
diropt = ""
echo("Apply the patch by running:")
Copy link
Contributor

Choose a reason for hiding this comment

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

why one and the other? THey seem to do the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure. @tuulos ^

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry, what's the question @romain-intel ?

@npow npow requested a review from romain-intel February 13, 2025 19:49
@npow
Copy link
Contributor Author

npow commented Feb 20, 2025

Superseded by #2282

@npow npow closed this Feb 20, 2025
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