-
Notifications
You must be signed in to change notification settings - Fork 13
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
Fix build jinja2 #88
Fix build jinja2 #88
Conversation
@will-moore @jburel : Maybe there was some larger plan about why not to go the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pwalczysko That's great - thanks. I started trying to fix this one way and ended up taking a more complex route, but this way is nice.
@jburel Do you want to merge please ? I think this would unblock @will-moore with minimal effort expended |
Conflicting PR. Removed from build OMERO-plugins-push#227. See the console output for more details.
--conflicts |
Conflicting PR. Removed from build OMERO-plugins-push#228. See the console output for more details.
--conflicts |
Hmmm - just looking at this again because of the conflicts. I don't think I really looked at this closely before as I realise now this is updating the installation dependencies so that anyone installing mapr will also install jinja2. This is not ideal as the app has no requirement for jinja2 - it's only needed for testing because the testing setup imports |
@@ -1,2 +1,3 @@ | |||
requests | |||
omero-web[redis]>=5.14.0 | |||
jinja2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds a dependency not required by the app
I have declared
as per #85 (comment).
@will-moore : Is there any reason why you went for the (imho much more complex) options in #87 ?