Feature: Add `--trackDir` option #72

ee7 · 2020-10-23T14:46:55Z

This commit allows the configlet binary to act on a different directory
than its own. This is useful in general, but it will also help us to
test configlet itself - we can now establish an initial state for
end-to-end tests by specifying:

a track directory
a problem-specifications directory
the offline mode

That allows us to do black-box testing of the release binary, running
something like:

    configlet sync -t=/tmp/python -p=/tmp/my_prob_specs -o -m=include

and then asserting that the changes made are as expected.

Note that we implement -t, --track-dir as a global option, rather than
an option for sync - it will also be useful for e.g. lint.

Some decisions:

Set the default value of conf.trackDir explicitly to the current
directory in cli.nim.
Determine the tests.toml path from conf.trackDir, but don't change
the current directory to conf.trackDir.

Closes: #50

WIP:

Rebase this PR on master and reflect recent changes (e.g. --trackDir -> --track-dir)
In sync mode, make it write to the given -t, --trackDir, rather than relative to the current directory.
Merge sync: refactor the writing of tests.toml #134 and rebase this PR on top.
Address review comments

Let's add tests in a later PR. We can leave -t, --track-dir undocumented in the first configlet releases if we feel it isn't tested well enough.

ErikSchierboom

Great! Approving pending the fix for the writing of the tests.toml files relative to the track dir.

ErikSchierboom · 2021-01-26T08:28:28Z

src/sync/sync.nim

@@ -100,13 +101,13 @@ proc sync(exercise: Exercise, mode: Mode): Exercise =

  result.tests = initExerciseTests(included, excluded, missing)

-  writeFile(result)
+  writeTestsToml(result, conf.trackDir)


Is conf.trackDir set to the current directory if the --track-dir argument is not specified?

It was set to the empty string, which ends up meaning the current directory.

From the examples in https://nim-lang.github.io/Nim/os.html#/,string,string

`assert "" / "lib" == "lib"`

So

trackDir / "exercises" / exercise.slug / ".meta" / "tests.toml"

was the same as

"exercises" / exercise.slug / ".meta" / "tests.toml"

when trackDir is empty.

I pushed a new commit, do you prefer it?

ErikSchierboom · 2021-01-26T08:34:58Z

src/sync/tracks.nim

+    if conf.trackDir.len > 0:
+      TrackRepo(dir: conf.trackDir)
+    else:
+      newTrackRepo()


I'm wondering if it would perhaps be easier to set conf.trackDir to the current directory if the --track-dir argument is specified.

set conf.trackDir to the current directory if the --track-dir argument is specified.

Do you mean "set the current directory to conf.trackDir if --track-dir is specified"?

We could indeed change the current directory in the program somewhere, but I think that might be harder to debug - it's more implicit. In particular, we might not want to change directory all the time when we're running the tests. It's also a problem that we have some instances of deep quit in the code right now.

We could use the withDir template, but it might be easier to just pass conf or conf.trackDir around for now. In the long-term I want to do a refactoring pass on exercises.nim, tracks.nim, and sync.nim that should help some of these issues. I have an old branch for that, but I never finished it.

With this commit, we can now establish a beginning state by specifying: - a track directory - a problem-specifications directory - the offline mode This allows us to do black-box testing of the release binary, running something like: configlet -t /tmp/python -p /tmp/my_prob_specs --offline and then asserting that the changes made are as expected. Closes: exercism#50

Previously the test file path was always derived from the working directory.

Previously `conf.trackDir` was the empty string when `--track-dir` was not provided. That worked previously because code like let myPath = "" / "foo" resolves to "foo" in the current directory, but setting it explicitly is more obvious.

ee7 · 2021-01-26T15:01:12Z

src/sync/tracks.nim

-proc newTrackRepo: TrackRepo =
-  result.dir = getCurrentDir()
-


I made this change partly because I find it more readable to inline it in findTrackExercises.

But there's also a general issue that I'll try to fix everywhere eventually: I believe that procs that initialize non-ref objects should be named starting with init, not new - even if they contain a field of a type like string.

From the style guide: https://nim-lang.github.io/Nim/nep1.html#introduction-naming-conventions

The stdlib API is designed to be easy to use and consistent. Ease of use is measured by the number of calls to achieve a concrete high level action. The ultimate goal is that the programmer can guess a name.

The library uses a simple naming scheme that makes use of common abbreviations to keep the names short but meaningful.

English word To use Notes

initialize initT init is used to create a value type T

new newP new is used to create a reference type P

... ... ...

For example, in the stdlib sets module, we write initHashSet for an object named HashSet, even though that object has a field of type seq

But in my refactoring branch I think I changed a lot of this stuff to use distinct string rather than an one-field object anyway.

ErikSchierboom

LGTM! The only thing I found was that when using --track-dir, the prob-specs repo gets cloned to the current directory instead of the track's dir. We can fix that later.

ee7 · 2021-01-26T16:11:22Z

when using --track-dir, the prob-specs repo gets cloned to the current directory instead of the track's dir. We can fix that later.

Yes, currently the temporary .problem-specifications folder is created in the current directory rather than the track dir. But that would only be visible to a user that does a keyboard interrupt or encounters an error, right?

We should be careful that everything behaves correctly when both options are provided, regardless of the order:

./configlet sync -t mytrack -p myprobspecs
./configlet sync -p myprobspecs -t mytrack
./configlet -t mytrack sync -p myprobspecs

ErikSchierboom · 2021-01-27T07:09:09Z

Yes, currently the temporary .problem-specifications folder is created in the current directory rather than the track dir. But that would only be visible to a user that does a keyboard interrupt or encounters an error, right?

Correct. It would be mostly for the error case that I would expect it to be located in the track dir.

This commit allows the configlet binary to act on a different directory than its own. This is useful in general, but it will also help us to test configlet itself - we can now establish an initial state for end-to-end tests by specifying: - a track directory - a problem-specifications directory - the offline mode That allows us to do black-box testing of the release binary, running something like: configlet sync -t=/tmp/python -p=/tmp/my_prob_specs -o -m=include and then asserting that the changes made are as expected. Note that we implement `-t, --track-dir` as a global option, rather than an option for `sync` - it will also be useful for e.g. `lint`. Some decisions: - Set the default value of `conf.trackDir` explicitly to the current directory in `cli.nim`. - Determine the `tests.toml` path from `conf.trackDir`, but don't change the current directory to `conf.trackDir`. Closes: exercism#50

ee7 requested a review from ErikSchierboom as a code owner October 23, 2020 14:46

ee7 marked this pull request as draft October 23, 2020 14:55

ErikSchierboom approved these changes Oct 27, 2020

View reviewed changes

ee7 force-pushed the feature-add-trackdir-option branch from d807ca3 to 5d7f540 Compare October 30, 2020 11:18

ee7 force-pushed the feature-add-trackdir-option branch from 5d7f540 to 64ae1f0 Compare January 26, 2021 00:22

This was referenced Jan 26, 2021

sync: refactor the writing of tests.toml #134

Merged

Rename repo to configlet #124

Closed

ErikSchierboom reviewed Jan 26, 2021

View reviewed changes

ee7 added 2 commits January 26, 2021 11:44

sync: support --track-dir

4f3ff45

Previously the test file path was always derived from the working directory.

ee7 force-pushed the feature-add-trackdir-option branch from 64ae1f0 to 4f3ff45 Compare January 26, 2021 10:46

ee7 commented Jan 26, 2021

View reviewed changes

ee7 marked this pull request as ready for review January 26, 2021 15:23

ErikSchierboom approved these changes Jan 26, 2021

View reviewed changes

ee7 merged commit 1721fd2 into exercism:master Jan 26, 2021

ee7 deleted the feature-add-trackdir-option branch January 26, 2021 21:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Add `--trackDir` option #72

Feature: Add `--trackDir` option #72

ee7 commented Oct 23, 2020 •

edited

Loading

ErikSchierboom left a comment

ErikSchierboom Jan 26, 2021

ee7 Jan 26, 2021 •

edited

Loading

ErikSchierboom Jan 26, 2021

ee7 Jan 26, 2021 •

edited

Loading

ee7 Jan 26, 2021 •

edited

Loading

ErikSchierboom left a comment

ee7 commented Jan 26, 2021

ErikSchierboom commented Jan 27, 2021

English word	To use	Notes
initialize	`initT`	`init` is used to create a value type `T`
new	`newP`	`new` is used to create a reference type `P`
...	...	...

Feature: Add --trackDir option #72

Feature: Add --trackDir option #72

Conversation

ee7 commented Oct 23, 2020 • edited Loading

ErikSchierboom left a comment

Choose a reason for hiding this comment

ErikSchierboom Jan 26, 2021

Choose a reason for hiding this comment

ee7 Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

ErikSchierboom Jan 26, 2021

Choose a reason for hiding this comment

ee7 Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

ee7 Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

ErikSchierboom left a comment

Choose a reason for hiding this comment

ee7 commented Jan 26, 2021

ErikSchierboom commented Jan 27, 2021

Feature: Add `--trackDir` option #72

Feature: Add `--trackDir` option #72

ee7 commented Oct 23, 2020 •

edited

Loading

ee7 Jan 26, 2021 •

edited

Loading

ee7 Jan 26, 2021 •

edited

Loading

ee7 Jan 26, 2021 •

edited

Loading