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 --bdctl-url option #56

Merged
merged 2 commits into from
May 6, 2020
Merged

Add --bdctl-url option #56

merged 2 commits into from
May 6, 2020

Conversation

ObserverOfTime
Copy link
Collaborator

@ObserverOfTime ObserverOfTime commented Nov 22, 2019

This option allows users to specify the URL used when upgrading betterdiscordctl.
It can be used to apply temporary or pre-release fixes. Example:

betterdiscordctl upgrade --bdctl-url \
  'https://raw.githubusercontent.com/bb010g/betterdiscordctl/temp-fix/betterdiscordctl'

I've also updated the path of the flatpak version (see #45 (comment))
and removed the redirection to FD 3 because it fails with sh: 3: Bad file descriptor.

Moreover, it doesn't seem like snap actually uses XDG_CONFIG_HOME so I've removed that.

@bb010g I believe these changes require a review.


This change is Reviewable

This also updates the location of the flatpak installation
Copy link
Owner

@bb010g bb010g left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ObserverOfTime)


betterdiscordctl, line 24 at r1 (raw file):

snap=
flatpak=
bdctl_url='https://git.io/bdctl'

I would really prefer if we didn't use this. URL shorteners aren't clear, and we're not hurting on bytes.


betterdiscordctl, line 60 at r1 (raw file):

      --bdctl-url=URL            Use the specified URL for upgrading BDCTL
                                 (default 'https://git.io/bdctl')

This can just be --upgrade-url=URL, "URL to upgrade this program with". Variables should be appropriately renamed.


betterdiscordctl, line 348 at r1 (raw file):

  if [[ $semver_diff -eq 1 ]]; then
    printf 'Downloading betterdiscordctl...\n' >&2
    if curl -LSso "$SOURCE" "$bdctl_url"; then

Keeping the -L is good, even without the shortener.


betterdiscordctl, line 400 at r1 (raw file):

          if [[ -z $modules ]]; then
            bdc_find_modules
          else # --modules

Please don't use trailing comments.


betterdiscordctl, line 428 at r1 (raw file):

    # shellcheck disable=SC2016
    # Expansion should happen inside snap's shell.
    xdg_config=$("$snap_bin" run --shell discord <<< 'echo $SNAP_USER_DATA/.config 1>&3' 3>&1)

The quotes should definitely be kept ($SNAP_USER_DATA could contain multiple spaces). This should probably be $'printf \'%s\\n\' "$SNAP_USER_DATA/.config" 1>&3' to avoid an echo that tries to parse a strange $SNAP_USER_DATA starting with - as an option.


README.md, line 102 at r1 (raw file):

  option argument will be used as the flatpak command to call.

* `--bdctl-url` (default `https://git.io/bdctl`)

Also update for --update-url.

Copy link
Collaborator Author

@ObserverOfTime ObserverOfTime left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @bb010g and @ObserverOfTime)


betterdiscordctl, line 24 at r1 (raw file):

Previously, bb010g (bb010g) wrote…

I would really prefer if we didn't use this. URL shorteners aren't clear, and we're not hurting on bytes.

I shortened it to avoid going over 80 characters in the CLI help text.


betterdiscordctl, line 400 at r1 (raw file):

Previously, bb010g (bb010g) wrote…

Please don't use trailing comments.

What's wrong with trailing comments?


betterdiscordctl, line 428 at r1 (raw file):

Previously, bb010g (bb010g) wrote…

The quotes should definitely be kept ($SNAP_USER_DATA could contain multiple spaces). This should probably be $'printf \'%s\\n\' "$SNAP_USER_DATA/.config" 1>&3' to avoid an echo that tries to parse a strange $SNAP_USER_DATA starting with - as an option.

echo handles multiple spaces just fine but you're right about -.

Shouldn't the command be $'printf -- \'%s/.config\\n\' "$SNAP_USER_DATA" 1>&3' though? And the same should be done for the other echos.

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.

2 participants