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 writeas config command #25

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Add writeas config command #25

wants to merge 1 commit into from

Conversation

a-mt
Copy link

@a-mt a-mt commented Jan 19, 2019

Adds the command "config", to be able to easily edit the user configurations.
I know you didn't ask for it, feel free to reject it.
If you don't, here's some reservations that I have:

  • I don't understand how the errors are handled

    • commands.go:

      return err
      
      InfolnQuit("msg")
      
      Errorln("msg")
      return cli.NewExitError("", 1)
      
      return cli.NewExitError("msg")
      
      ErrorlnQuit("msg")
      
    • fonts.go:

      fmt.Print("msg")
      
    • options.go:

      Info(c, "msg")
      
    • posts.go:

      if debug {
        panic(err)
      } else {
        Errorln("msg")
        return
      }
      
      return fmt.Errorf("msg")
      

    Basically, I don't understand in which cases errors should be returned rather than printed, and with which method :/

  • When the user doesn't give the right options / arguments:
    Shouldn't we print the command usage and its options, using cli.ShowSubcommandHelp(c) (the same output as writeas COMMAND --help) and putting the usage in the field UsageText (cli.go), rather than calling cli.NewExitError("usage: writeas ...", 1)?

Looking forward to your feedback!

@ghost
Copy link

ghost commented Jun 18, 2019

Hi @a-mt, this is actually something that is being discussed over on our phabricator https://phabricator.write.as/T596

The errors were a bit all over, there have been a lot of changes since you opened this - sorry about that. There will likely be more changes needed to clean things up further as well.

If you are still interested in working on this let us know, and maybe come by our phabricator to discuss details.

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.

None yet

1 participant