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

Proof-of-concept for "snow ws" command generation #1540

Closed
wants to merge 78 commits into from

Conversation

sfc-gh-cgorrie
Copy link
Contributor

@sfc-gh-cgorrie sfc-gh-cgorrie commented Sep 6, 2024

Pre-review checklist

  • I've confirmed that instructions included in README.md are still correct after my changes in the codebase.
  • I've added or updated automated unit tests to verify correctness of my new code.
  • I've added or updated integration tests to verify correctness of my new code.
  • I've confirmed that my changes are working by executing CLI's commands manually on MacOS.
  • I've confirmed that my changes are working by executing CLI's commands manually on Windows.
  • I've confirmed that my changes are up-to-date with the target branch.
  • I've described my changes in the release notes.
  • I've described my changes in the section below.

Changes description

Generates snow ws @<entity_id> command groups based on the current directory's snowflake.yml.

What's done:

  • allows decoupling commands from classes, so that each one can live in its own compilation unit
  • defines an annotation DSL (e.g. HelpText, ParameterDeclarations) that ensures individual commands don't actually rely on click / typer
  • generates click / typer commands from those annotations
  • allows commands like "version create" to automatically create sub-trees

Things that need addressing:

  • deal with position action arguments (e.g. snow @pkg version create v1 needs to be passed in instead as --version v1)
  • tests are failing
  • commands cannot share option arguments (they all need to re-define them)

return

root = dm.unrendered_project_definition
for (target_id, model) in root.entities.items():
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider model.get_command_group(target_id) that would encapsulate L100-102? WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I quite understand the suggestion. Can you give a bit more detail?

Base automatically changed from cgorrie-context-refactor to main September 10, 2024 17:21
pass_through_options = {
k: v for (k, v) in options.items() if k in known_params
}
ws.perform_action(self.target_id, action, **pass_through_options)
Copy link
Contributor

Choose a reason for hiding this comment

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

how do we handle return values? for example, we currently have some command that wrap the return value of the entity action in MessageResult, others use QueryResult, etc

Comment on lines +142 to +144
return MessageResult(
f"Successfully performed {action.verb} on {self.target_id}."
)
Copy link
Contributor

Choose a reason for hiding this comment

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

not all actions want to show a success message (especially those that retrieve data, like version-list)

@sfc-gh-fcampbell
Copy link
Contributor

Closing this for now due to uncertainty regarding the snow ws command. We can use the ideas from this PR in a future re-attempt if necessary.

@sfc-gh-fcampbell sfc-gh-fcampbell deleted the cgorrie-uber-command branch October 2, 2024 15:55
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