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

Fixes #617: Add simple template processor in jar :file. #618

Closed
wants to merge 1 commit into from

Conversation

jgrzebyta
Copy link

  • accepts version and project templates

Signed-off-by: Jacek Grzebyta jgrzebyta@users.noreply.github.com

- accepts version and project

Signed-off-by: Jacek Grzebyta <jgrzebyta@users.noreply.github.com>
@Deraen
Copy link
Contributor

Deraen commented Sep 26, 2017

Lein uses format for these, and only supports using version: :jar-name "hello-%.jar".
Maven supports properties: <file>foo-${foo.version}</file>.

I think copying Lein behaviour would be sensible, unless there is a good reason to also support project name?

@jgrzebyta
Copy link
Author

jgrzebyta commented Sep 26, 2017 via email

@alandipert
Copy link
Contributor

I think we're not really comparable to either lein or maven in this case, since we're programmable. The user can easily fill a template string in their build.boot based on strings they define and pass as arguments to tasks, which could also be the strings used to generate the pom.

That said, I see no reason not to do this, even if its arguably going against the grain in our model. It's backward compatible and can save some typing. 👍

@Deraen if you are OK with it too please feel free to merge when you see this.

Copy link
Member

@martinklepsch martinklepsch left a comment

Choose a reason for hiding this comment

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

I don't like it too much that this goes "against the grain" as Alan already noted but the complexity is within reason so I'm ok to merge it.

There are some potential NPEs if I'm not mistaken, with some particularly bad error messages if the user makes a typo in his pattern. It would be great to see those addressed before merging 🙂

@@ -846,7 +846,8 @@
(boot.pom/pom-xml-parse-string ~(slurp pom))))
pomname (when (and project version)
(str (name project) "-" version ".jar"))
fname (or file pomname "project.jar")
file-parsed-string (string/replace file #"\{(\w+)\}" (fn [[_ group]] ((keyword group) {:project (name project) :version version})))
Copy link
Member

Choose a reason for hiding this comment

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

(string/replace nil ,,,)

This will throw, no?

Copy link
Member

Choose a reason for hiding this comment

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

The replacement function seems a bit hard to read and will throw if an unknown pattern is provided.
Maybe something like the following could be used?

(fn [[_ group]]
  (case group 
    "project" (name project)
    "version" version
    (do (util/warn "Unknown pattern {%s} in jar task's :file option\n" group)
        (str "{" group "}")))))

This will provide a helpful message if the user makes a typo instead of this:

NullPointerException   java.util.regex.Matcher.quoteReplacement (Matcher.java:701)

For readability the replacement function could also be put into it's own local var or even in a top-level defn-.

@martinklepsch
Copy link
Member

@jgrzebyta are you planning to get back to this or should it be closed? 🙂

@jgrzebyta
Copy link
Author

@martinklepsch I will close this PR.

@jgrzebyta jgrzebyta closed this Jan 13, 2018
@martinklepsch
Copy link
Member

Ok! Thanks for your efforts! 💛

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.

4 participants