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

Allow key '{git-commit}' for source URIs #170

Merged
merged 1 commit into from
Jun 9, 2018

Conversation

UweHubert
Copy link

README.md Outdated
* `{classpath}` - the relative path of the file within the source directory
* `{line}` - the line number of the source file
* `{version}` - the version of the project
* `{git-commit-id}` - the Git commit id of the repository
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps change this to git-commit instead. It saves a few characters, and it doesn't add ambiguity in context.

@@ -91,17 +93,41 @@
(apply text/read-documents)
(sort-by :name))))

(defn- git-commit-id [dir]
Copy link
Owner

Choose a reason for hiding this comment

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

This function tries to avoid exceptions, but this is the opposite of what we should be doing. Instead:

(defn git-commit [dir]
  (let [{:keys [out exit err] :as result} (shell/sh "git" "rev-parse" "HEAD" :dir dir)]
    (when (neg? exit)      (throw (ex-info "Error getting git commit"    result)
    (when (str/blank? out) (throw (ex-info "Git commit number not found" result)
    (str/trim out)))

Copy link
Author

Choose a reason for hiding this comment

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

Checking 'exit' is probably sufficient.

:exclude-vars #"^(map)?->\p{Upper}"
:metadata {}
:themes [:default]
:git-commit-id (git-commit-id root-path)}))
Copy link
Owner

Choose a reason for hiding this comment

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

Let's change this to :git-commit and then place it in a delay so that repositories without a git project don't error.

(str/replace "{basename}" (uri-basename path))
(str/replace "{line}" (str line))
(str/replace "{version}" version)
(str/replace "{git-commit-id}" git-commit-id))))
Copy link
Owner

Choose a reason for hiding this comment

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

If git-commit is a delay, we can deref or force it here.

@weavejester
Copy link
Owner

Thanks for the patch. I added some review comments. Can you also ensure that your commit message follows the seven rules?

@UweHubert
Copy link
Author

Thank you for the comments. 'git-commit' is now a delay and only dereferenced if needed. It fails hard if we are not in a git repo.

@@ -91,17 +93,25 @@
(apply text/read-documents)
(sort-by :name))))

(defn git-commit [dir]
Copy link
Owner

Choose a reason for hiding this comment

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

Let's make this private.

(-> uri
uri (if (map? source-uri) (get-source-uri source-uri path) source-uri)
gc "{git-commit}"
uri* (cond-> uri
Copy link
Owner

Choose a reason for hiding this comment

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

We should avoid using cond-> for compatibility.

uri (if (map? source-uri) (get-source-uri source-uri path) source-uri)
gc "{git-commit}"
uri* (cond-> uri
(and (string? uri) (.contains uri gc)) (str/replace gc @git-commit))]
Copy link
Owner

Choose a reason for hiding this comment

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

It would be better to factor this out into a function. So:

(defn- force-replace [^String s match replacement]
  (if (.contains s match)
    (str/replace match (force replacement))
    s))

@@ -14,7 +14,7 @@
:doc-files ["doc/intro.md"
"doc/formatting.md"]
:source-uri
"https://github.com/weavejester/codox/blob/{version}/codox.example/{filepath}#L{basename}-{line}"
"https://github.com/weavejester/codox/blob/{git-commit}/example/{filepath}#L{basename}-{line}"
Copy link
Owner

Choose a reason for hiding this comment

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

We should add this to a profile so that the {version} key is still tested.

In source URIs the key '{git-commit}' is replaced with the Git commit
id of the repository.

See weavejester#168
@UweHubert
Copy link
Author

Applied requested changes and removed "{git-commit}" from build.boot.

@UweHubert UweHubert changed the title Added the placeholder {git-commit-id} for the :source-uri. Added the placeholder {git-commit} for the :source-uri. Feb 9, 2018
@UweHubert UweHubert changed the title Added the placeholder {git-commit} for the :source-uri. Allow key '{git-commit}' for source URIs Feb 9, 2018
@weavejester weavejester merged commit f1f7e16 into weavejester:master Jun 9, 2018
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.

2 participants