Optional bits for giftwrap #8

Closed
mathstuf opened this Issue Feb 2, 2017 · 8 comments

Comments

2 participants
@mathstuf

mathstuf commented Feb 2, 2017

I like the idea, but it'd be nice if some things were optional, such as rustfmt (there are still bugs with it for some patterns; I need to collect them and file bugs), CI (not everyone uses CI that has artifacts in the repo, so some way to say "done externally" would be great), and potentially other things (not everyone uses git, so .gitignore).

@killercup

This comment has been minimized.

Show comment
Hide comment
@killercup

killercup Feb 2, 2017

Owner

I actually want cargo giftwrap to be pretty opinionated. I don't want it to have a configuration as it's meant to be simple enforce conventions. It is of course not a requirement to follow every one of its suggestions if you feel like you know better.

How about making things conditional instead? I didn't really write about it, but here is what I imagine:

  • If there is a .git/config with ≥ 1 remotes, then the repository field in Cargo.toml needs to be set to one of these remotes
  • If the repository is set to a public Github URL, a known CI tool needs to be used.
  • If a CI tool is used and its badge can be set in Cargo.toml, it needs to be set in Cargo.toml

And, more controversially:

  • If you don't want to use rustfmt, you need to add #![rustfmt_skip] to you crate
Owner

killercup commented Feb 2, 2017

I actually want cargo giftwrap to be pretty opinionated. I don't want it to have a configuration as it's meant to be simple enforce conventions. It is of course not a requirement to follow every one of its suggestions if you feel like you know better.

How about making things conditional instead? I didn't really write about it, but here is what I imagine:

  • If there is a .git/config with ≥ 1 remotes, then the repository field in Cargo.toml needs to be set to one of these remotes
  • If the repository is set to a public Github URL, a known CI tool needs to be used.
  • If a CI tool is used and its badge can be set in Cargo.toml, it needs to be set in Cargo.toml

And, more controversially:

  • If you don't want to use rustfmt, you need to add #![rustfmt_skip] to you crate
@mathstuf

This comment has been minimized.

Show comment
Hide comment
@mathstuf

mathstuf Feb 2, 2017

Those sound good to me.

And I do use rustfmt, but I do not apply all of its changes (they're usually issues with indentation mismatches, all of them bugs).

mathstuf commented Feb 2, 2017

Those sound good to me.

And I do use rustfmt, but I do not apply all of its changes (they're usually issues with indentation mismatches, all of them bugs).

@killercup

This comment has been minimized.

Show comment
Hide comment
@killercup

killercup Feb 2, 2017

Owner
Owner

killercup commented Feb 2, 2017

killercup added a commit that referenced this issue Feb 2, 2017

@mathstuf

This comment has been minimized.

Show comment
Hide comment
@mathstuf

mathstuf Feb 2, 2017

Yeah, but this is kind of stuff I'm talking about:

Annoying, the arguments are related:

     let commit = ctx.git()
         .arg("commit")
         .arg("--allow-empty")
-        .arg("-m").arg("root commit")
+        .arg("-m")
+        .arg("root commit")
         .output()
         .unwrap();

bugs:

             result.add_error(format!("commit {} is a known-bad commit that was removed from the \
                                       server.",
-                                     commit.sha1_short))
+                                   commit.sha1_short))

mathstuf commented Feb 2, 2017

Yeah, but this is kind of stuff I'm talking about:

Annoying, the arguments are related:

     let commit = ctx.git()
         .arg("commit")
         .arg("--allow-empty")
-        .arg("-m").arg("root commit")
+        .arg("-m")
+        .arg("root commit")
         .output()
         .unwrap();

bugs:

             result.add_error(format!("commit {} is a known-bad commit that was removed from the \
                                       server.",
-                                     commit.sha1_short))
+                                   commit.sha1_short))
@killercup

This comment has been minimized.

Show comment
Hide comment
@killercup

killercup Feb 2, 2017

Owner

Yeah, I agree. The second one should be

result.add_error(format!(
    "commit {sha} is a known-bad commit that was removed from the server.",
    sha = commit.sha1_short,
))

or I'm not gonna be happy…

(Aside from that, you should probably use the git2 crate instead of git's cli. Oh, and can you use .args(&["-m", "root commit"]) or something like that?)

Owner

killercup commented Feb 2, 2017

Yeah, I agree. The second one should be

result.add_error(format!(
    "commit {sha} is a known-bad commit that was removed from the server.",
    sha = commit.sha1_short,
))

or I'm not gonna be happy…

(Aside from that, you should probably use the git2 crate instead of git's cli. Oh, and can you use .args(&["-m", "root commit"]) or something like that?)

@mathstuf

This comment has been minimized.

Show comment
Hide comment
@mathstuf

mathstuf Feb 2, 2017

libgit2 doesn't do allow all the things we need that the git command can do (at least it couldn't last I checked).

mathstuf commented Feb 2, 2017

libgit2 doesn't do allow all the things we need that the git command can do (at least it couldn't last I checked).

@mathstuf

This comment has been minimized.

Show comment
Hide comment
@mathstuf

mathstuf Feb 2, 2017

Yeah, no git update-index equivalent in the C API that I see.

mathstuf commented Feb 2, 2017

Yeah, no git update-index equivalent in the C API that I see.

@killercup

This comment has been minimized.

Show comment
Hide comment
@killercup

killercup Mar 9, 2017

Owner

(No activity for ≥30 days, closing)

Owner

killercup commented Mar 9, 2017

(No activity for ≥30 days, closing)

@killercup killercup closed this Mar 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment