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.

Sign up for GitHub

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

Jump to bottom

Change programs' interfaces #17

Merged
merged 6 commits into from
Nov 15, 2018
Merged

Change programs' interfaces #17

merged 6 commits into from
Nov 15, 2018

Conversation

skalee
Copy link
Contributor

@skalee skalee commented Nov 7, 2018

For this option, --at is a better name than --version, as --version conventionally means "print program version and exit".

This is a breaking change.

cc: @ronaldtse, @dewyatt.

@ronaldtse ronaldtse requested a review from dewyatt November 7, 2018 15:40
@skalee
Copy link
Contributor Author

skalee commented Nov 7, 2018 •
edited
Loading

See also #14.

@dewyatt
Copy link
Contributor

dewyatt commented Nov 8, 2018

Not a big deal but what do you think about something more descriptive like --component-version? I just think --at isn't very intuitive.

@ronaldtse
Copy link
Contributor

Agreed.

@skalee
Copy link
Contributor Author

skalee commented Nov 8, 2018

--component-version is fine. I'll also rename --component to --component-name for consistency.

@skalee
Copy link
Contributor Author

skalee commented Nov 8, 2018 •
edited
Loading

The only doubt I have is related to using --component-version together with --git option. In such case, --component-version should be a Git branch or tag. A more ambiguous --at was more suitable for that.

However, maybe we change approach here, and define yet another option --component-git-ref which would be used instead of --component-version in this case? I think it's a good idea. I'll add some commits to address that.

@skalee skalee force-pushed the rename-argument branch 2 times, most recently from 5155f0e to 361d8de Compare November 8, 2018 18:50
@skalee skalee removed the request for review from dewyatt November 8, 2018 18:51
@skalee
Copy link
Contributor Author

skalee commented Nov 8, 2018 •
edited
Loading

Please treat this pull request as a work-in-progress for now.

@dewyatt
Copy link
Contributor

dewyatt commented Nov 13, 2018

The only doubt I have is related to using --component-version together with --git option. In such case, --component-version should be a Git branch or tag. A more ambiguous --at was more suitable for that.

However, maybe we change approach here, and define yet another option --component-git-ref which would be used instead of --component-version in this case? I think it's a good idea. I'll add some commits to address that.

I don't really see any issues with having --git --component-version gnupg-2.2.11, --git --component-version fe621cc, --component-version latest all be valid uses.
Adding another git-specific option adds unnecessary complexity IMO, I don't think we need a --component-git-ref.

@skalee
Copy link
Contributor Author

skalee commented Nov 13, 2018

I have implemented this change already, and IMO it makes things bit cleaner. I can revert it though if someone doesn't like it.

@skalee
Rename command argument
f7605e7 
--component-version is a better name than --version, as --version
conventionally means "print program version and exit".
@skalee
Rename command argument
1020ea2 
--component-name is a better name than just --component, as it is more
descriptive.  But most importantly, it is consistent with a recently
added --component-version.
@skalee
Implement new option in "install component" script
64919ae 
Introduce option --component-git-ref which supersedes --git option,
and replaces --component-version in context of former --git option.

In other words, --component-git-ref REF replaces the sequence of
--git --component-version REF options.

This change is expected to simplify program interface, make option
names more obvious and consistent, and reduce confusion.
@skalee
Consistent error handling
934a1ed
@skalee
Rewrite interface of "install all" script
cb24f37 
Write command arguments parser similar to one in
install_gpg_component.sh script.
@skalee
Document recent interface changes
f23860d
@ronaldtse
Copy link
Contributor

@skalee the build is failing. Does it matter?

@skalee
Copy link
Contributor Author

skalee commented Nov 13, 2018

@ronaldtse It is out of scope of this pull request. Nevertheless, these failures are not false positives. Fixes will be included in further pull requests.

@skalee skalee mentioned this pull request Nov 13, 2018
Open
@skalee skalee changed the title Rename command argument Change programs' interfaces Nov 13, 2018
@skalee
Copy link
Contributor Author

skalee commented Nov 13, 2018

In install_gpg_component.sh script:

  • option --component has been renamed to --component-name
  • option --version has been renamed to --component-version
  • option --git has been removed in favour of --component-git-ref

In install_gpg_all.sh script:

  • option -i has been renamed to --suite-version, and fixed
  • other options except for --help have been removed
  • code style has been improved

Documentation has been updated for both scripts.

Fixes #14.

@skalee skalee requested a review from dewyatt November 13, 2018 20:16
dewyatt
dewyatt approved these changes Nov 15, 2018
View reviewed changes
Copy link
Contributor

@dewyatt dewyatt left a comment

Choose a reason for hiding this comment

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

LGTM, the CI issues can be addressed in subsequent PRs, but just noting here that IIRC gpg 2.1's executable name is gpg2 by default (configurable), so the which gpg check won't really work

@ronaldtse ronaldtse self-requested a review November 15, 2018 03:20
ronaldtse
ronaldtse approved these changes Nov 15, 2018
View reviewed changes
Copy link
Contributor

@ronaldtse ronaldtse left a comment

Choose a reason for hiding this comment

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

Thanks @skalee !

@ronaldtse ronaldtse merged commit cb3704b into master Nov 15, 2018
@ronaldtse ronaldtse deleted the rename-argument branch November 15, 2018 03:58
This was referenced Nov 19, 2018
Closed
Closed
skalee added a commit that referenced this pull request Mar 28, 2019
@skalee
Update README for --suite-version option
b655d63 
Script interfaces have been changed in
#17,
however documentation was only partly updated.  This commit fixes that.
skalee added a commit that referenced this pull request Mar 28, 2019
@skalee
Update and slightly restructure README (#34)
5fc16eb 
* Update README for --suite-version option

Script interfaces have been changed in
#17,
however documentation was only partly updated.  This commit fixes that.

* Slightly reorganize README

Introduce "Tips & tricks" section, in which some most important options
and use cases can be described in detail.

* Slightly improve "Prerequisites" section in README

* Improve description of scripts

* Split a very long line in README
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronaldtse ronaldtse ronaldtse approved these changes

@dewyatt dewyatt dewyatt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@skalee @dewyatt @ronaldtse