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

--probSpecsDir option: Cope with ssh-cloned repo #78

Merged
merged 2 commits into from
Oct 27, 2020
Merged

--probSpecsDir option: Cope with ssh-cloned repo #78

merged 2 commits into from
Oct 27, 2020

Conversation

glennj
Copy link
Contributor

@glennj glennj commented Oct 23, 2020

Repo cloned with ssh does not match github.com/exercism

$ cd ../problem-specifications/
$ git remote -v
origin	git@github.com:exercism/problem-specifications.git (fetch)
origin	git@github.com:exercism/problem-specifications.git (push)

@glennj glennj changed the title Cope with ssh-cloned repo --probSpecsDir option: Cope with ssh-cloned repo Oct 23, 2020
Copy link
Member

@ee7 ee7 left a comment

Choose a reason for hiding this comment

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

Thanks.

We should update the doc comment (and probably the error message) to reflect that location no longer contains github.com, but otherwise I think this PR should be merged.

I'd like to improve the scanf pattern instead of using two contains, but I can do that in another PR. I see the possibilities as:

  1. hardcode github.com (removing host as a parameter) or
  2. pass host as a static string and then write something like:
  var remoteName, remoteUrl, x: string # `x` to ignore http(s)/git/www.
  for line in remotes.splitLines():
    if line.scanf(&"$s$w$s$+{host}$+(fetch)$.", remoteName, x, remoteUrl):
      if remoteUrl.continuesWith(location, start = 1): # ignore ':' and '/'
        return remoteName

src/probspecs.nim Outdated Show resolved Hide resolved
src/probspecs.nim Outdated Show resolved Hide resolved
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
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.

I've verified that it still works for HTTPS repos. The change looks looks to me. I'll wait for the approve of @ee7 to merge.

@ErikSchierboom ErikSchierboom merged commit 5ae4d5a into exercism:master Oct 27, 2020
@ErikSchierboom
Copy link
Member

Thanks @glennj!

@glennj glennj deleted the problem-spec-remote-check branch October 27, 2020 15:01
ee7 added a commit to ee7/exercism-configlet that referenced this pull request Jan 21, 2021
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
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.

3 participants