killercup / scribbles

Some notes on various topics.
https://deterministic.space
64 stars 11 forks source link

Optional bits for giftwrap #8

Closed mathstuf closed 7 years ago

mathstuf commented 7 years ago

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 commented 7 years ago

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:

And, more controversially:

mathstuf commented 7 years ago

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 commented 7 years ago

I personally hate visual indentation for most things 😄

Am 02.02.2017 um 15:00 schrieb Ben Boeckel notifications@github.com:

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).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

mathstuf commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 7 years ago

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

killercup commented 7 years ago

(No activity for ≥30 days, closing)