Skip to content

Move add-hook to activate function. #1

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kasz
Copy link
Collaborator

@kasz kasz commented Feb 12, 2013

lein2 reports problems without this patch.

@michalmarczyk
Copy link
Owner

Thanks! The tests seem to fail with this applied, though. Does it actually seem to work for you on a real project?

@kasz
Copy link
Collaborator Author

kasz commented Feb 20, 2013

Yes, it works correctly with leiningen 2. I am using it currenlty. You are right about tests though - this poses problem when someone uses this version with leiningen 1.x. I will submit patch shortly.

@michalmarczyk
Copy link
Owner

Cool, thanks. Actually I was running the tests (and seeing them fail) in lein2 previously, but your new commit makes them happy with both lein1 and lein2, so that's great. I've added two minor tweaks @ https://github.com/michalmarczyk/lein-git-version/tree/1-add-hook-in-activate-contd (got rid of the version range in project.clj and moved all activation logic into activate -- tests still pass); if this seems to work for you, I'd call it 0.0.9.

(Now it would also be cool to figure out why the individual hooks get run multiple times, so that e.g. version.txt gets created three times over -- happily it's no big deal here, is it?)

@kasz
Copy link
Collaborator Author

kasz commented Feb 21, 2013

Your version looks better, since it avoids code duplication. Problem is that in this scenario activate function is run two times in lein2. Granted, it is not a serious problem.
Meanwhile I've noticed another problem - deploy task in lein2 does not deploy jar with tag name but uses version name contained in project.clj. I'm going to push solution soon.
I think that this patch should be also integrated into 0.0.9.

@kasz
Copy link
Collaborator Author

kasz commented Feb 21, 2013

Here it is.

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