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

fetch-configlet: print instructions for installing completions #647

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ee7
Copy link
Member

@ee7 ee7 commented Aug 6, 2022

Closes: #635


@glennj I've taken the liberty of taking your commit from glennj@e55d775 to begin this PR, which should be attributed to you.

The first commit in this PR is just that commit, but rebased on main. Feel free to push to this PR's branch.

This PR is a follow-up to #629, where these changes were originally.

glennj and others added 5 commits August 6, 2022 11:49
Format with:

    shfmt -i 2 -ci -sr -kp

`shfmt` usage:

    $ shfmt --version
    3.5.1
    $ shfmt --help
    usage: shfmt [flags] [path ...]

    shfmt formats shell programs. If the only argument is a dash ('-') or no
    arguments are given, standard input will be used. If a given path is a
    directory, all shell scripts found under that directory will be used.

      --version  show version and exit

      -l,  --list      list files whose formatting differs from shfmt's
      -w,  --write     write result to file instead of stdout
      -d,  --diff      error with a diff when the formatting differs
      -s,  --simplify  simplify the code
      -mn, --minify    minify the code to reduce its size (implies -s)

    Parser options:

      -ln, --language-dialect str  bash/posix/mksh/bats, default "auto"
      -p,  --posix                 shorthand for -ln=posix
      --filename str               provide a name for the standard input file

    Printer options:

      -i,  --indent uint       0 for tabs (default), >0 for number of spaces
      -bn, --binary-next-line  binary ops like && and | may start a line
      -ci, --case-indent       switch cases will be indented
      -sr, --space-redirects   redirect operators will be followed by a space
      -kp, --keep-padding      keep column alignment paddings
      -fn, --func-next-line    function opening braces are placed on a separate line

    Utilities:

      -f, --find   recursively find all shell files and print the paths
      --tojson     print syntax tree to stdout as a typed JSON

    For more information, see 'man shfmt' and https://github.com/mvdan/sh.
@ee7
Copy link
Member Author

ee7 commented Aug 6, 2022

It's cool that we try to detect a user's shell. But I have these questions:

  1. Is there a way to reliably detect the user's current shell when it is different from their default shell?
  2. And if not, what do we think about "don't try to detect the user's shell, and just print the instructions for every supported shell"?

For example, a user might use fish as an interactive shell only, as discussed on the Arch wiki. Right now, for such a user,

getent passwd "${USER}" | cut -d: -f7

may return

/bin/bash

in which case the user sees instructions for a different shell. Or it may return, say:

/bin/zsh

in which case the user wouldn't see any instructions for installing completions, even though they're available.

Related recent post of Glenn's on unix.stackexchange.com:

$SHELL is the user's login shell. My login shell is fish: if I unset the SHELL variable and then launch bash, SHELL is /path/to/fish

@ErikSchierboom
Copy link
Member

And if not, what do we think about "don't try to detect the user's shell, and just print the instructions for every supported shell"?

If there isn't a reliable, fool-proof way to detect the user's shell, I think we should print the instructions for every supported shell.

@glennj
Copy link
Contributor

glennj commented Aug 18, 2022

  1. Is there a way to reliably detect the user's current shell when it is different from their default shell?

No, unless we parse ps to find the shell in the process hierarchy.

  1. And if not, what do we think about "don't try to detect the user's shell, and just print the instructions for every supported shell"?

I think this was my original suggestion: let the user tell you what shell they want:

configlet completions -s fish

Perhaps if the -s option is not provided, list the available shells for which we have completions.

@ee7
Copy link
Member Author

ee7 commented Aug 18, 2022

If there isn't a reliable, fool-proof way to detect the user's shell, I think we should print the instructions for every supported shell.

Let's do that.

No, unless we parse ps to find the shell in the process hierarchy.

Hah. I think that'd be trying to be much too clever.

I think this was my original suggestion: let the user tell you what shell they want:

configlet completions -s fish

We'll have this in the next configlet release (but with the subcommand called completion, not completions - see d10dcd6).

The help message will contain

Usage:
  configlet [global-options] <command> [command-options]

Commands:
  completion  Output a completion script for a given shell
  fmt         Format the exercise '.meta/config.json' files
  generate    Generate Concept Exercise 'introduction.md' files from 'introduction.md.tpl' files
  info        Print some information about the track
  lint        Check the track configuration for correctness
  sync        Check or update Practice Exercise docs, metadata, and tests from 'problem-specifications'.
              Check or populate missing 'files' values for Concept/Practice Exercises from the track 'config.json'.
  uuid        Output new (version 4) UUIDs, suitable for the value of a 'uuid' key

Options for completion:
  -s, --shell <shell>          Choose the shell type (required)
                               Allowed values: b[ash], f[ish], z[sh]

[...]

So the main remaining step is to make fetch-configlet explain how to install completions. I'd like for us to explain how to install the bash and zsh completions on a not-just-for-this-session basis.

I think this PR doesn't block the configlet release, though.

@ee7 ee7 mentioned this pull request Aug 18, 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.

fetch-configlet: detect user's shell and print instructions
3 participants