Add git initialization to `new` command #320

LoipesMas · 2022-10-19T20:21:17Z

Closes #281
Added initialization of basic git repo to the new command. Can be skipped with --no-vcs. Adds git2 dependency, as discussed in the issue.
I considered moving the init call to create_project, but that didn't feel right.

cnpryer · 2022-10-19T21:01:17Z

Thanks! Catching this on mobile. I'll review later.

cnpryer

Only comment is that I'd think we'd want the initialize logic in ops, but I can check that out another time if you don't feel up to making that change here.

Curious why you didn't think it would make sense to do that.

cnpryer · 2022-10-20T00:35:38Z

Would be able to maybe add a simple little test there too.

LoipesMas · 2022-10-20T21:00:18Z

I was thinking that create_project is for creating "huak-project" and git is kinda separate to that. So I wanted to avoid polluting that function with vcs, which is more like an add-on, rather than an integral part of the huak-project. But looking at it now it doesn't make as much sense. Maybe something like ops::new::init_vcs would fit this best? Or maybe I should just stick it in the create_project, since it's just one call to git lib (at least for now)

dpgraham4401 · 2022-10-20T21:13:56Z

@LoipesMas without looking at this too closely, seems like create_project is the primary function of ops::new, so it should roughly encapsulate the business logic of setting up the user's project, not just initializing a struct/object. If it's just one if-else clause, I would say that's business logic and goes in create_project function. That can get refactored out later, but I tend to go by the Go Proverb "A little copy, is better than a little dependency"

So to test the interface and not the implementation, we could test 2 scenarios; 1. default (no option flag passed, init new project with .git) and 2. no git flag.

In both, I would think just adjust the create_project parameters, test that the CliResults are as expected, and the .git directory exists(?)

of course, I haven't looked at this yet. So maybe disregard me for the time being LOL

Oh crap, you're totally right, create_project pretty much just initializes a Project Struct. Welp, forget everything I said.

dpgraham4401 · 2022-10-20T21:50:29Z

@LoipesMas I think we're going to start following the MVC design paradigm in the future. I've been mocking up kind of what that looks like, to me at least, here.

But at the end of the day, this is @cnpryer brain-baby, so he has emperor Palpatine style ultimate power.

cnpryer · 2022-10-21T01:49:28Z

ops::new::init_vcs would fit this best

Seems sensible to me.

LoipesMas · 2022-10-21T15:17:38Z

But at the end of the day, this is @cnpryer brain-baby, so he has emperor Palpatine style ultimate power.

Yeah, I think it's reasonable to have someone with more decisive power in this scenario. I don't have the full view of the project and often it's more of an issue of keeping things consistent rather than finding objectively best solution.
So I'll do as @cnpryer commands, unless I'll have a good reason not to.

cnpryer · 2022-10-21T16:16:14Z

No need for the commander vibes! I always appreciate wise advice. Don't be afraid to disagree. With that said I do agree with the point about consistency. I think over time as things stabilize we'll have less issues with that.

The goals in the readme are supposed to drive that consistency. So any feedback on clarity there is appreciated as well.

LoipesMas · 2022-10-21T18:13:58Z

Don't be afraid to disagree.

Oh, I won't be! ;]

LoipesMas · 2022-10-21T18:18:47Z

I moved the git init to function in opts and added tests, but one thing I wasn't sure about was whether that new function or the command function should handle the conditionality. Passing an argument to the function that can make the function do completely nothing seems wrong. But with the condition checking in command function I had to test the whole command, which seems like something we do not do.

cnpryer

but one thing I wasn't sure about was whether that new function or the command function should handle the conditionality.

I think of it as the bin space is a separate application altogether than depends on our lib and the ops implementations. So calling init_vcs from the bin space (CLI app) is correct imo.

cnpryer · 2022-10-21T19:06:11Z

src/bin/huak/commands/new.rs

+#[cfg(test)]
+mod tests {
+    use tempfile::tempdir;
+
+    #[test]
+    fn initializes_git() {
+        let directory = tempdir().unwrap().into_path();
+        std::env::set_current_dir(&directory).unwrap();
+        super::run("foo".to_owned(), true, false, false).unwrap();
+        let proj_dir = directory.join("foo");
+        assert!(proj_dir.is_dir());
+        assert!(proj_dir.join(".git").is_dir());
+    }
+
+    #[test]
+    fn does_not_initialize_git() {
+        let directory = tempdir().unwrap().into_path();
+        std::env::set_current_dir(&directory).unwrap();
+        super::run("foo".to_owned(), true, false, true).unwrap();
+        let proj_dir = directory.join("foo");
+        assert!(proj_dir.is_dir());
+        assert!(!proj_dir.join(".git").exists());
+    }
+}


We should test in the ops space.

The issue here is that we can test init_vcs from ops space, but we can't test there whether the argument passed to command::new::run is being correctly used, which is what these tests test (in addition to init_vcs, but that's a side effect).

And I'm not sure what needs to be tested and what doesn't. Personally I'm not a big fan of unit testing, especially granular/specific ones. So, for me, a more general test that covers a bigger part of the functionality is good enough. But to each their own. And I don't mind writing the tests themselves, it's just that I don't feel where the coverage line should be drawn.

but we can't test there whether the argument passed to command::new::run is being correctly used

This is a separate issue that we haven't solved yet for every other command as well. The tradeoff is manageable while the CLI app logic is super light. I've tinkered with testing through clap usage that I'd like to introduce here at some point. That's when I think testing in the bin space will be appropriate.

Personally I'm not a big fan of unit testing, especially granular/specific ones

Hmm interesting. I'm the opposite. I like tons of unit-level coverage.

it's just that I don't feel where the coverage line should be drawn.

I personally don't see an issue with over-optimizing for more coverage (making the tradeoff of redundancy if needed). But I don't think we're at a point where we need to worry about any redundant testing.

I would compare premature testing to premature optimization. Unless it's some critical part, you probably don't need it tested. In this situation, even if something went wrong, worse case scenario is a git repo is not initialized. Which would be pretty easy to notice and then to debug.
IMO it's a waste of developers' time, when writing tests and then when refactoring (and not even including conversations like this :P ). But then again, of all the things to "waste" time on, tests are not so bad.

IMO requiring contributions to be paired with sufficient testing isn't more than what should be expected. I don't believe tests are ever premature. The earlier on the simpler the tests (in theory). If what you are contributing is simple but requires significant effort to test, then I'd reconsider the implementation.

It's, imo, similar to choosing to using a more complicated programming language like Rust. You make an investment early on that feels like it's time potentially better spent elsewhere, but the ROI is worth it since that investment pays off with better stability, more correct code, etc..

It's roughly the same idea. The tests may seem like overkill today, but as we start adding more and more in, we'll be happy we have that coverage when we want to sustain our pace of development without breaking existing logic. Whether or not the coverage is related, the piece of mind alone is a huge win.

Also I'm a little confused how this is related to moving the tests to ops::new 😅

We went on a bit of a tangent here.

Also I'm a little confused how this is related to moving the tests to ops::new

Barely at all. I just haven't been at my workstation to do the change until now. Done now.

To end the discussion: We have to draw a line somewhere. I would draw it in a different spot, but I accept where you draw it.

cnpryer · 2022-10-22T14:53:46Z

LGTM ty!

Add git initialization to new command

008a788

cnpryer reviewed Oct 20, 2022

View reviewed changes

Little refactor

35bc12c

Add tests

28a9079

cnpryer reviewed Oct 21, 2022

View reviewed changes

Move test to ops

acb77f8

cnpryer merged commit 3027fb7 into cnpryer:master Oct 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add git initialization to `new` command #320

Add git initialization to `new` command #320

LoipesMas commented Oct 19, 2022

cnpryer commented Oct 19, 2022

cnpryer left a comment •

edited

cnpryer commented Oct 20, 2022

LoipesMas commented Oct 20, 2022

dpgraham4401 commented Oct 20, 2022 •

edited

dpgraham4401 commented Oct 20, 2022

cnpryer commented Oct 21, 2022 •

edited

LoipesMas commented Oct 21, 2022

cnpryer commented Oct 21, 2022

LoipesMas commented Oct 21, 2022

LoipesMas commented Oct 21, 2022

cnpryer left a comment

cnpryer Oct 21, 2022

LoipesMas Oct 21, 2022

cnpryer Oct 21, 2022 •

edited

LoipesMas Oct 21, 2022

cnpryer Oct 21, 2022 •

edited

cnpryer Oct 21, 2022

LoipesMas Oct 22, 2022

cnpryer commented Oct 22, 2022

Add git initialization to new command #320

Add git initialization to new command #320

Conversation

LoipesMas commented Oct 19, 2022

cnpryer commented Oct 19, 2022

cnpryer left a comment • edited

Choose a reason for hiding this comment

cnpryer commented Oct 20, 2022

LoipesMas commented Oct 20, 2022

dpgraham4401 commented Oct 20, 2022 • edited

dpgraham4401 commented Oct 20, 2022

cnpryer commented Oct 21, 2022 • edited

LoipesMas commented Oct 21, 2022

cnpryer commented Oct 21, 2022

LoipesMas commented Oct 21, 2022

LoipesMas commented Oct 21, 2022

cnpryer left a comment

Choose a reason for hiding this comment

cnpryer Oct 21, 2022

Choose a reason for hiding this comment

LoipesMas Oct 21, 2022

Choose a reason for hiding this comment

cnpryer Oct 21, 2022 • edited

Choose a reason for hiding this comment

LoipesMas Oct 21, 2022

Choose a reason for hiding this comment

cnpryer Oct 21, 2022 • edited

Choose a reason for hiding this comment

cnpryer Oct 21, 2022

Choose a reason for hiding this comment

LoipesMas Oct 22, 2022

Choose a reason for hiding this comment

cnpryer commented Oct 22, 2022

Add git initialization to `new` command #320

Add git initialization to `new` command #320

cnpryer left a comment •

edited

dpgraham4401 commented Oct 20, 2022 •

edited

cnpryer commented Oct 21, 2022 •

edited

cnpryer Oct 21, 2022 •

edited

cnpryer Oct 21, 2022 •

edited