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

Update tool commands #117

Merged
merged 3 commits into from
Nov 17, 2022
Merged

Update tool commands #117

merged 3 commits into from
Nov 17, 2022

Conversation

github-actions[bot]
Copy link
Contributor

Automated commits for 37b8a4f created by CML.

@rogermparent rogermparent temporarily deployed to gatsby-theme-main-cml-p-o1zorl November 10, 2022 10:06 Inactive
Copy link
Contributor

@yathomasi yathomasi 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 to me. Mind taking another look @jorgeorpinel

@jorgeorpinel
Copy link

Nice. LGTM but we'd need to ask the product teams to confirm these are the publicly available commands now cc @dberenbaum
@casperdcl @aguschin

@jorgeorpinel jorgeorpinel removed their request for review November 15, 2022 23:15
@yathomasi
Copy link
Contributor

we'd need to ask the product teams to confirm these are the publicly available commands now

Yes, they are already public. This PR is generated based on the main/master branch of each respective repo.

@rogermparent
Copy link
Contributor

rogermparent commented Nov 17, 2022

Nothing looks very out of place to me, though I can't say I have a great base to confirm. If nothing else, the subcommands seem to all be in the right order!

@yathomasi yathomasi temporarily deployed to gatsby-theme-main-cml-p-o1zorl November 17, 2022 10:27 Inactive
@yathomasi yathomasi merged commit 9c5b0ef into main Nov 17, 2022
@yathomasi yathomasi deleted the main-cml-pr-37b8a4fe branch November 17, 2022 10:31
Comment on lines +4 to +8
'pr',
'comment',
'check',
'workflow',
'tensorboard'
Copy link
Contributor

@casperdcl casperdcl Nov 21, 2022

Choose a reason for hiding this comment

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

are subcommands meant to be included? If so...

Suggested change
'pr',
'comment',
'check',
'workflow',
'tensorboard'
'runner launch',
'pr',
'pr create',
'comment',
'comment create',
'comment update',
'check',
'check create',
'workflow',
'workflow rerun',
'tensorboard'
'tensorboard connect'

Also not sure if order is important, i.e.:

Suggested change
'pr',
'comment',
'check',
'workflow',
'tensorboard'
'runner launch',
'runner',
'pr create',
'pr',
'comment create',
'comment update',
'comment',
'check create',
'check',
'workflow rerun',
'workflow',
'tensorboard connect'
'tensorboard'

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we would want subcommands as well to highlight them properly.
Can we do it organize the subcommands similarly to dvc.org (eg: dvc data status ) to ensure consistency and also it will provide better navigation on the sidebar?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unlike DVC, atm it doesn't make sense for CML subcommands to have separate pages.

i.e. https://dvc.org/doc/command-reference/exp/init and https://dvc.org/doc/command-reference/exp/run are big enough to be on separate pages, but https://cml.dev/doc/ref/comment#create and https://cml.dev/doc/ref/comment#update should be on the same page - trying to separate them would harm reader experience.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we ensure consistency, it will be picked up by the GHA automation.

create and update should be on the same page - trying to separate them would harm reader experience.

This seems to be a compromise we can work on(linking each other with a little more description) if we want to use the automation to generate the commands list upon addition.

In a long run, we want to have these commands listed on the consumer project themself instead of on the theme project (#134).

Copy link
Contributor

@casperdcl casperdcl Nov 25, 2022

Choose a reason for hiding this comment

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

what do you mean by "the GHA automation" - can you provide a link to some code/logic?

Copy link
Contributor

Choose a reason for hiding this comment

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

This PR was generated by https://github.com/iterative/gatsby-theme-iterative/blob/main/.github/workflows/update-commands.yml.
The script basically gets all the commands from the sidebar.json from the respective tool's repository and updates the command list.

@casperdcl casperdcl mentioned this pull request Nov 25, 2022
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