Ran into some issues getting started about spr HOT 8 CLOSED

Thanks for taking the time to respond to all this stuff. I kind of just dumped a wall of text on you there.

You're most welcome. Thanks again for raising these issues.

• the list of requirements looks about right: libgit2 is fundamental to what spr does. libiconv would be needed for unicode handling. openssl is probably needed to make GitHub API requests, to connect via https. Are any of those problematic in particular, or are you just generally interested in keeping the number of dependencies low?

Sorry, I should've explained a bit more. I'm not particularly concerned about which dependencies are needed nor how many of them there are. Whether you trim down the dependencies is up to you. It might make it easier to get installed (maybe also maintain), but they all seem fairly standard for what's going on here. It's more that I had to figure it out by trial and error. The ask (that I forgot to actually ask for sweat_smile) is only to document the dependencies rather than change them in some way.

Ah, now I get it. I assume you installed a binary from https://github.com/getcord/spr/releases - I kind of forgot we had those. I set up those build before I did the homebrew formula. I am tempted to stop making them and refer Mac users to homebrew - because that obviously handles the dependency management. Or do you personally prefer installing a gzipped binary? (I am not a Mac user myself, so I try to gauge if there is use for having the binary releases here in addition to homebrew).

On that note, I did go through some dependencies and deselected some unused features, as well as switched to rustls. Future binaries should not depend on openssl anymore.

• I am not sure how spr should interact with pull request templates, as writing the messages is not done in spr. We use git config commit.template and point it to a checked-in file (.gitmessage.template) to have a shared template for out git commit messages (which then become pull request descriptions). This could be a good solution, especially if spr was more configurable in its handling of commit messages.

Now that you mention it, I'm not entirely sure it would be a useful experience if the authoring of commits is done outside of spr. I was kind of thinking that spr would use the PR template to validate the commit message when doing spr diff, but that would mean parsing and working with markdown (which can be pretty hard). If you could configure the required sections somehow, that's probably good enough. Though, it might still need to be able to at least parse sections differently Test Plan: vs. ## Test Plan vs. some other thing.

That sounds like a good idea. Will have to think about the details. Maybe spr could be configured to produced either plain-text-style or markdown-style commit message. The parser could of course understand both "Test plan:" and "## Test plan", but spr would consistently produce the preferred flavour.

• good point about spr land and requiring reviews. I think we can just skip this check, actually. If you configure GitHub to require accept to merge, then it should refuse spr's merge API command if the PR wasn't accepted.

I will say about this one that I kind of do appreciate the idea of spr doing its own validation on whether a PR can land or not. Maybe if there's configuration that happens for spr this could be one of the things that could be configured irrespective of what the settings are on GitHub.

Why not. The next release will have a evaluate the spr.requireApproval config, which will default to false.

I am also working on adding more/better documentation. Plus, a bug caused by flaky GitHub behaviour that occasionally left the pull request un-updateable by spr is getting fixed.

So stay tuned! 😉

from spr.

@joneshf - I am trying to get spr into nixpkgs, see NixOS/nixpkgs#178168

from spr.

Hi @joneshf! Thanks a ton for giving spr a spin and all your feedback!

Let me go through the list of issues, you found:

• the list of requirements looks about right: libgit2 is fundamental to what spr does. libiconv would be needed for unicode handling. openssl is probably needed to make GitHub API requests, to connect via https. Are any of those problematic in particular, or are you just generally interested in keeping the number of dependencies low?
• the dependency on openssl is introduced by the reqwest library, in order to be able to make encrypted connections to the GitHub API. Having said that, we can configure the build to use rustls instead.
• two to-dos here: first improve documentation to mention and explain the "Test plan" section. The second would be to allow users to configure what sections they want in the commit message, and be able to make them optional or required. As of now, this configuration is hard-coded and insists on a test plan. (This is how the two Phabricator instances that I happened to have worked with were set up.) I am not sure how spr should interact with pull request templates, as writing the messages is not done in spr. We use git config commit.template and point it to a checked-in file (.gitmessage.template) to have a shared template for out git commit messages (which then become pull request descriptions). This could be a good solution, especially if spr was more configurable in its handling of commit messages.
• good point about spr land and requiring reviews. I think we can just skip this check, actually. If you configure GitHub to require accept to merge, then it should refuse spr's merge API command if the PR wasn't accepted.
• we could query what version of git is installed, and not include the --no-write-fetch-head if it's not available. That means spr might overwrite your FETCH_HEAD file if you're on an older version of git, but that's probably better than not working at all.
• will fix the spr init help to include the right scopes!
• will fix abort-by-empty-message!
• I have to research what to do about GPG. We don't use it. GitHub signs the squash-merge commits, so in our git history each commit is signed by GitHub, but we haven't bothered with GPG for signing commits that you submit for code review.
• Sections are not git trailers, and I don't think they should be. They are not meant to be metadata key-value pairs, but just part of structured commit messages (i.e. part of the commit message payload).
• closing PRs is also a good idea. I have just done that via the GitHub ui.

Wow, that's a lot of stuff. Thanks again for all this feedback. I can quickly fix a few of them, but will open issues for the remaining ones (citing your issue).

from spr.

Thanks for taking the time to respond to all this stuff. I kind of just dumped a wall of text on you there.

• the list of requirements looks about right: libgit2 is fundamental to what spr does. libiconv would be needed for unicode handling. openssl is probably needed to make GitHub API requests, to connect via https. Are any of those problematic in particular, or are you just generally interested in keeping the number of dependencies low?

Sorry, I should've explained a bit more. I'm not particularly concerned about which dependencies are needed nor how many of them there are. Whether you trim down the dependencies is up to you. It might make it easier to get installed (maybe also maintain), but they all seem fairly standard for what's going on here. It's more that I had to figure it out by trial and error. The ask (that I forgot to actually ask for 😅) is only to document the dependencies rather than change them in some way.

• I am not sure how spr should interact with pull request templates, as writing the messages is not done in spr. We use git config commit.template and point it to a checked-in file (.gitmessage.template) to have a shared template for out git commit messages (which then become pull request descriptions). This could be a good solution, especially if spr was more configurable in its handling of commit messages.

Now that you mention it, I'm not entirely sure it would be a useful experience if the authoring of commits is done outside of spr. I was kind of thinking that spr would use the PR template to validate the commit message when doing spr diff, but that would mean parsing and working with markdown (which can be pretty hard). If you could configure the required sections somehow, that's probably good enough. Though, it might still need to be able to at least parse sections differently Test Plan: vs. ## Test Plan vs. some other thing.

• Sections are not git trailers, and I don't think they should be. They are not meant to be metadata key-value pairs, but just part of structured commit messages (i.e. part of the commit message payload).

Makes sense. I wasn't entirely sure, so I appreciate the explanation!

from spr.

• good point about spr land and requiring reviews. I think we can just skip this check, actually. If you configure GitHub to require accept to merge, then it should refuse spr's merge API command if the PR wasn't accepted.

I will say about this one that I kind of do appreciate the idea of spr doing its own validation on whether a PR can land or not. Maybe if there's configuration that happens for spr this could be one of the things that could be configured irrespective of what the settings are on GitHub.

from spr.

• the list of requirements looks about right: libgit2 is fundamental to what spr does. libiconv would be needed for unicode handling. openssl is probably needed to make GitHub API requests, to connect via https. Are any of those problematic in particular, or are you just generally interested in keeping the number of dependencies low?

Sorry, I should've explained a bit more. I'm not particularly concerned about which dependencies are needed nor how many of them there are. Whether you trim down the dependencies is up to you. It might make it easier to get installed (maybe also maintain), but they all seem fairly standard for what's going on here. It's more that I had to figure it out by trial and error. The ask (that I forgot to actually ask for sweat_smile) is only to document the dependencies rather than change them in some way.

Ah, now I get it. I assume you installed a binary from https://github.com/getcord/spr/releases - I kind of forgot we had those. I set up those build before I did the homebrew formula. I am tempted to stop making them and refer Mac users to homebrew - because that obviously handles the dependency management. Or do you personally prefer installing a gzipped binary? (I am not a Mac user myself, so I try to gauge if there is use for having the binary releases here in addition to homebrew).

On that note, I did go through some dependencies and deselected some unused features, as well as switched to rustls. Future binaries should not depend on openssl anymore.

I think I'm a bit of an outlier. I don't actually have brew installed on this macOS machine. I tend to use nix to manage my dependencies. So, I dunno if I'm the one you want to respond to your questions. I imagine most people on macOS would use brew.

from spr.

Thanks for adding that!

from spr.

Looks like everything except the git version one has been addressed or has an issue tracking it. I'm going to turn that last one into an issue and close this since it will be tracked there.

from spr.