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

Simplify the gathering of host information. #561

Conversation

gabrieldemarmiesse
Copy link
Collaborator

@gabrieldemarmiesse gabrieldemarmiesse commented Aug 2, 2019

I proposed a simplified version of the host info gathering. With this, we'll have less code to maintain and it'll be easier to understand for futur contributors.

There is also no need for the dictionary used to gather infos to be exposed at the top level, since it's undocumented, I removed it.

@coveralls
Copy link

coveralls commented Aug 2, 2019

Coverage Status

Coverage decreased (-0.03%) to 85.294% when pulling 979075e on gabrieldemarmiesse:simplify_info_gathering into a181df5 on IDSIA:master.

@Qwlouse
Copy link
Collaborator

Qwlouse commented Aug 3, 2019

Hmm, not sure about this. The idea behind the current implementation was to make it easy to customize host-info from the user-level. I.e. if you want some custom information captured, just decorate a function with @host_info_getter(name='custom') and its results will be added to host_info.

On the other hand I can see the advantages of reducing complexity, and your code is clearly much simpler and more maintainability.
@JarnoRFB: Opinions?

@JarnoRFB
Copy link
Collaborator

JarnoRFB commented Aug 3, 2019

I wouldn't necessarily break the user facing API for an internal simplification. The functionality also seems to be documented here https://sacred.readthedocs.io/en/latest/collected_information.html#host-info. Unless we can find a way to simplify and keep the feature, I would leave it.

@gabrieldemarmiesse
Copy link
Collaborator Author

I didn't see it was a user facing API. My bad.

@gabrieldemarmiesse gabrieldemarmiesse deleted the simplify_info_gathering branch August 5, 2019 12:05
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.

None yet

4 participants