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.

Sign up for GitHub

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

Jump to bottom

[#182] Fix compatibility with hamster-lib 0.13 #183

Merged
merged 2 commits into from
Jun 19, 2017
Merged

[#182] Fix compatibility with hamster-lib 0.13 #183

merged 2 commits into from
Jun 19, 2017

Conversation

jtojnar
Copy link
Member

@jtojnar jtojnar commented Jun 7, 2017 •
edited by elbenfreund
Loading

This PR updates our dependencies to require hamster-lib>=0.13.0. As this introduces backwards incompatible changes with regards to the config helper, some additional compatibility fixes are included as well.

Closes: #182

@jtojnar
Copy link
Member Author

jtojnar commented Jun 7, 2017 •
edited
Loading

The changes also broke raw facts without a start time, that should probably be fixed in hamster-lib. See projecthamster/hamster-lib@f8b16cf#diff-dcf70660b8bbb2102574bb4e2f86556dR345-R347

elbenfreund
elbenfreund reviewed Jun 13, 2017
View reviewed changes
Copy link
Collaborator

@elbenfreund elbenfreund left a comment

Choose a reason for hiding this comment

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

This commit does in fact do two things:

  1. swap 'hamster-gtk's get_config_instance in favour of the new hamster-lib.config_helpers.load_config_file
  2. Move the entire app namespace from something hamster-gtk specific to the generic projecthamster namespace.

the 1st change is unproblematic.

The 2nd change is something I understand the reasoning very well and ponder how to do this properly for a while now. However, at the very least it should be a separate PR as this does not really relate to the changes to hamster-lib (other than that it proves default values).

I personally always thought that "app specific" namespaces are just as valid as a generic "projecthamster" one-for-all... I can see reasons for both.
In light of this I always envisioned that once we get there we provide a flag that can be passed at program invocation that governs where the program looks for its data and config. This should not be too complicated but I did not document so far...

@jtojnar
Copy link
Member Author

jtojnar commented Jun 13, 2017

I can revert to the hamster-gtk namespace for now, I actually prefer it.

@elbenfreund
Copy link
Collaborator

Oh, and about the time parsing: this most likely should be a separate issue...

@jtojnar jtojnar force-pushed the feature/182-lib-013-compat branch from 1518692 to e044e3f Compare June 13, 2017 12:18
elbenfreund
elbenfreund reviewed Jun 13, 2017
View reviewed changes
Copy link
Collaborator

@elbenfreund elbenfreund left a comment

Choose a reason for hiding this comment

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

In order to avoid instantiating several HamsterAppDir instances via self._get_appdir maybe it would be better to do this once in the App instance and just reuse it like so app._appdir?

@jtojnar jtojnar force-pushed the feature/182-lib-013-compat branch from e044e3f to 61f2e7e Compare June 13, 2017 12:51
@elbenfreund elbenfreund self-requested a review June 19, 2017 10:29
elbenfreund
elbenfreund previously approved these changes Jun 19, 2017
View reviewed changes
jtojnar and others added 2 commits June 19, 2017 12:33
@jtojnar @elbenfreund
Catch up with hamster-lib 0.13 config helper changes
aa55a04 
``hamster-lib 0.13.0`` changes the interface of the configuration helpers
in a backwards incompatible way. The now unnecessary helper was also removed.

Closes: #182
@elbenfreund
Fix broken link in docs
d2e87f4
@elbenfreund elbenfreund changed the title Fix compatibility with hamster-lib 0.13 [#182] Fix compatibility with hamster-lib 0.13 Jun 19, 2017
@elbenfreund elbenfreund added this to the 0.12.0 milestone Jun 19, 2017
@elbenfreund elbenfreund force-pushed the feature/182-lib-013-compat branch from 15e43b6 to d2e87f4 Compare June 19, 2017 10:34
@elbenfreund elbenfreund merged commit d2e87f4 into develop Jun 19, 2017
@jtojnar jtojnar deleted the feature/182-lib-013-compat branch June 19, 2017 10:43
@elbenfreund elbenfreund mentioned this pull request Jun 22, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elbenfreund elbenfreund elbenfreund approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.12.0
Development

Successfully merging this pull request may close these issues.

Doesn't run with latest hamster-lib (0.13.0)
2 participants
@jtojnar @elbenfreund