Skip to content
This repository has been archived by the owner on Dec 7, 2020. It is now read-only.

examples: unpinned dependencies #17

Merged
merged 1 commit into from
Mar 9, 2016
Merged

examples: unpinned dependencies #17

merged 1 commit into from
Mar 9, 2016

Conversation

jirikuncar
Copy link
Member

Signed-off-by: Jiri Kuncar jiri.kuncar@cern.ch

@jirikuncar jirikuncar added this to the Sprint-Record-Data-Model milestone Mar 7, 2016
tiborsimko
tiborsimko reviewed Mar 7, 2016
View reviewed changes
examples/app.py
$ cdvirtualenv src/invenio-marc21
$ pip install -e .[all]
$ pip install "invenio-theme>=1.0.0a9"
$ pip install "invenio-assets>=1.0.0a4"
Copy link
Member

Choose a reason for hiding this comment

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

What about introducing an upper limit as well in case the themes or assets develop more widely than marc21 example app? We've been bitten by this in Invenio 2.0 that is not changing much anymore... inveniosoftware/invenio#3604

Copy link
Member Author

Choose a reason for hiding this comment

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

IMHO examples are never going to be released on PyPI and should always work with current version. But of course I can add an upper limit. What do you propose - <=1.1 or <=2.0?

Copy link
Member

Choose a reason for hiding this comment

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

should always work with current version

I fully agree, however the example app is currently not CI-tested. If it were, then the upper version limit wouldn't be needed.

In other words, we could address my concern simply by generalising the example app testing, as we did here:

Note that problems already started to appear in other packages:

<=1.1

I thought about '<1.1' following semantic versioning. This would be akin to saying: "The example app is not tested during CI, but I tested it manually, and it works well with foo-1.7.x, bar-2.3.y".

@switowski
Copy link
Member

Also the pidstore should be added to the dependencies (as in my PR https://github.com/inveniosoftware/invenio-marc21/pull/11/files#diff-86d2f29b7151f1dfb6b158f5f495ba35R37).

@jirikuncar
Copy link
Member Author

@switowski I have added tests for example app. If you see any problem please let me know.

@switowski
Copy link
Member

@jirikuncar good idea with tests for the app! The code LGTM (I see that the pidstore was actually added in one of the commits), I will merge it after travis gives the 💚

@jirikuncar jirikuncar force-pushed the example-unpin branch 4 times, most recently from f04770c to 06d2fb9 Compare March 9, 2016 08:05
@switowski switowski mentioned this pull request Mar 9, 2016
Closed
@jirikuncar
Copy link
Member Author

@switowski :shipit:

@tiborsimko tiborsimko assigned tiborsimko and unassigned switowski Mar 9, 2016
@tiborsimko
Copy link
Member

  • When testing the example app, npm tries to write to the system-wide /usr/lib and /usr/bin places. Could be changed perhaps?
  • mock is needed for tests and is missing in the test requirement list. Can you add it?

Otherwise LGTM.

@tiborsimko tiborsimko assigned jirikuncar and unassigned tiborsimko Mar 9, 2016
@jirikuncar
Copy link
Member Author

  • When testing the example app, npm tries to write to the system-wide /usr/lib and /usr/bin places. Could be changed perhaps?

I don't know how.

  • mock is needed for tests and is missing in the test requirement list. Can you add it?

Added.

@jirikuncar jirikuncar assigned tiborsimko and unassigned jirikuncar Mar 9, 2016
@tiborsimko
Copy link
Member

Thanks. One more thing: let's add tests/test.db to .gitignore.

@jirikuncar
examples: unpinned dependencies
bc0c892 
Signed-off-by: Jiri Kuncar <jiri.kuncar@cern.ch>
@jirikuncar
Copy link
Member Author

Thanks. One more thing: let's add tests/test.db to .gitignore.

IMHO it is not related with this PR but I have added.

@tiborsimko
Copy link
Member

IMHO it is not related with this PR but I have added.

Yeah, it's just a clean-up for the past PRs... likemock, hence together. Thanks.

@tiborsimko tiborsimko merged commit bc0c892 into inveniosoftware:master Mar 9, 2016
@jirikuncar jirikuncar deleted the example-unpin branch March 9, 2016 14:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@tiborsimko tiborsimko

Labels
None yet
Projects
None yet
Milestone
Sprint-Record-Data-Model
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jirikuncar @switowski @tiborsimko