Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Add --probSpecsDir option #65

Merged
merged 4 commits into from
Oct 23, 2020

Conversation

ee7
Copy link
Member

@ee7 ee7 commented Oct 21, 2020

See the commit messages for more information - the commits are atomic.

Some things like the git error messages could probably be cleaned up a bit, and there's some obvious tests to add, but it should be reasonably robust. I think the implementation here is quite strict. Let me know if I missed any edge cases.

WIP:

  • Don't checkout or merge, even if the working directory is clean.
  • Try not to assume things about the remotes in the given directory.

Closes: #47

@ee7 ee7 requested a review from ErikSchierboom as a code owner October 21, 2020 23:08
@ee7 ee7 force-pushed the feature-add-probSpecsDir-option branch 2 times, most recently from 6cb97e5 to 2833c2b Compare October 22, 2020 12:59
@ee7 ee7 force-pushed the feature-add-probSpecsDir-option branch from 2833c2b to 1de7059 Compare October 23, 2020 09:16
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments! Excellent work.

tests/test_probspecs.nim Show resolved Hide resolved
src/probspecs.nim Outdated Show resolved Hide resolved
src/probspecs.nim Outdated Show resolved Hide resolved
@ee7 ee7 force-pushed the feature-add-probSpecsDir-option branch 2 times, most recently from ec650a9 to 0aafa75 Compare October 23, 2020 11:51
src/probspecs.nim Outdated Show resolved Hide resolved
src/probspecs.nim Outdated Show resolved Hide resolved
ee7 and others added 4 commits October 23, 2020 14:15
We want to add support for specifying the path of a
`problem-specifications` directory to use. However, the previous
workaround for `grains` was implemented by overwriting the
`canonical-data.json` file for `grains`.

It's better if we don't alter a user's non-temporary
`problem-specifications` repo, so this commit changes the `grains`
workaround to happen at the time of parsing instead.
The user can now tell the program to use an existing
`problem-specifications` directory, in which case the program does a
`fetch` rather than making a temporary shallow clone.

This is one of the biggest possible speed optimisations, given that the
program running time in non-interactive mode is limited by the time
taken to establish an up-to-date `problem-specifications`directory.
However, in the future we could add an `--offline` option (or similar).

This commit also makes it easier to add tests.

Decisions:
- Show an error if the given `problem-specifications` working directory
  is not clean, rather than asking the user or stashing changes.
- Try to be strict about the user's `problem-specifications` directory,
  and do not silently `checkout` or `merge`, at least for now.
- Don't assume the user has a remote named 'upstream'.
- Allow the given `problem-specifications` repo to be a shallow clone:
  don't check that it's actually a `problem-specifications` repo by
  requiring it to contain some specified SHA-1.

Closes: exercism#47

Co-authored-by: Erik Schierboom <[email protected]>
This commit adds our first tests.

Let's start with some basic checking of `findProbSpecsExercises`, which
is the main exported proc in `src/probspecs.nim`. We check the default
behaviour of creating a temporary shallow clone, as well as the
behaviour of the `--probSpecsDir` option in the simplest case.
@ee7 ee7 force-pushed the feature-add-probSpecsDir-option branch from 0aafa75 to aa15896 Compare October 23, 2020 12:21
@ErikSchierboom ErikSchierboom merged commit 24d52da into exercism:master Oct 23, 2020
@ee7 ee7 deleted the feature-add-probSpecsDir-option branch October 23, 2020 12:26
ee7 added a commit to ee7/exercism-configlet that referenced this pull request Oct 23, 2020
ErikSchierboom pushed a commit that referenced this pull request Oct 23, 2020
This should have been included in 6c603ae (#65).
ee7 added a commit to ee7/exercism-configlet that referenced this pull request Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sync: add flag providing path to non-transient problem-specifications repo
2 participants