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

[breaking] uniform cli commands and flag #1542

Merged
merged 23 commits into from
Nov 19, 2021
Merged

Conversation

umbynos
Copy link
Contributor

@umbynos umbynos commented Nov 9, 2021

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • What kind of change does this PR introduce?

refactor CLI commands and flags to be consistent with each other.

  • What is the current behavior?

close #1508

  • What is the new behavior?

This PR includes various improvements:

  • sketchpath calculation have been factored out

  • fqbn and programmer flags now have the same principle of operation as port flag

  • factored out a function that reports errors when conflicting flags are used

  • move vars on top and remove structs for uniformity

  • part of the init section in update-index commands have been factored out

  • other minor improvements

  • I also added logrus.Info() in every run function of every command

  • remove board details <fqbn> in favour of board details -b <fqbn>

  • replace board attach <port|fqbn> <sketch-path> with board attach -b <fqbn> | -p <port> <sketh-path>

  • change --timeout flag to --discovery-timeout in board list command for consistency

  • Does this PR introduce a breaking change, and is
    titled accordingly?

yep, documented in UPGRADING.md

  • Other information:

See how to contribute

@umbynos umbynos added type: enhancement Proposed improvement topic: code Related to content of the project itself topic: CLI Related to the command line interface labels Nov 9, 2021
@umbynos umbynos self-assigned this Nov 9, 2021
@umbynos umbynos requested a review from a team November 9, 2021 16:41
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

Since this is a big PR that contains multiple atomic commits, I recommend it be granted an exemption from the repo's usual squash merge policy.

This would be done by temporarily adjusting the configuration of the repo's merge button, following this procedure:

  1. Click the "⚙️ Settings" tab of the repository that contains the PR
  2. Under the "Merge button" section of the Settings page, check the box next to "🔲 Allow merge commits"
  3. Merge the PR, selecting the "Merge pull request" option from the merge button
  4. Return to the repository's Settings page
  5. Uncheck the box next to "🔲 Allow merge commits"

Since the commits will not be automatically squashed, any fixup commits should be manually squashed via an interactive rebase before the merge.

cli/arguments/port.go Outdated Show resolved Hide resolved
cli/arguments/sketch.go Outdated Show resolved Hide resolved
cli/debug/debug.go Show resolved Hide resolved
cli/arguments/sketch.go Outdated Show resolved Hide resolved
cli/arguments/port.go Outdated Show resolved Hide resolved
cli/arguments/port.go Outdated Show resolved Hide resolved
cli/arguments/sketch.go Outdated Show resolved Hide resolved
cli/arguments/sketch.go Outdated Show resolved Hide resolved
cli/board/details.go Show resolved Hide resolved
cli/instance/instance.go Outdated Show resolved Hide resolved
@umbynos umbynos force-pushed the umbynos/flags_refactoring branch 4 times, most recently from 2edafc0 to 80be710 Compare November 11, 2021 15:00
@umbynos umbynos changed the title uniform cli commands and flag [breaking] uniform cli commands and flag Nov 11, 2021
docs/UPGRADING.md Outdated Show resolved Hide resolved
docs/UPGRADING.md Outdated Show resolved Hide resolved
cli/board/attach.go Outdated Show resolved Hide resolved
docs/UPGRADING.md Outdated Show resolved Hide resolved
docs/UPGRADING.md Outdated Show resolved Hide resolved
docs/UPGRADING.md Outdated Show resolved Hide resolved
docs/UPGRADING.md Outdated Show resolved Hide resolved
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

Great stuff Umberto. Thanks!

cli/arguments/fqbn.go Outdated Show resolved Hide resolved
cli/arguments/fqbn.go Outdated Show resolved Hide resolved
docs/UPGRADING.md Outdated Show resolved Hide resolved
docs/UPGRADING.md Outdated Show resolved Hide resolved
Copy link
Contributor

@silvanocerza silvanocerza left a comment

Choose a reason for hiding this comment

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

See comments.

@umbynos umbynos force-pushed the umbynos/flags_refactoring branch 2 times, most recently from fb73cf8 to 61dba74 Compare November 18, 2021 14:47
@umbynos umbynos merged commit 9c13e87 into master Nov 19, 2021
@umbynos umbynos deleted the umbynos/flags_refactoring branch November 19, 2021 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: CLI Related to the command line interface topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Factor CLI argument processing for flags that are common across commands
3 participants