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

Maintenance/drop include #87

Merged
merged 9 commits into from
Mar 23, 2021
Merged

Maintenance/drop include #87

merged 9 commits into from
Mar 23, 2021

Conversation

maxfischer2781
Copy link
Member

@maxfischer2781 maxfischer2781 commented Mar 22, 2021 •
edited
Loading

This PR replaces the outdated include package with the stdlib importlib.

Closes #82.

See also #85 since this is a candidate for deprecation in the long run.

@maxfischer2781 maxfischer2781 marked this pull request as ready for review March 22, 2021 18:47
@maxfischer2781 maxfischer2781 requested review from a team, giffels and rcaspart and removed request for a team March 22, 2021 18:47
@maxfischer2781
Copy link
Member Author

CI is failing due to outdated formatting. This is fixed in #86.

rcaspart
rcaspart reviewed Mar 23, 2021
View reviewed changes
Copy link
Member

@rcaspart rcaspart left a comment

Choose a reason for hiding this comment

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

Thanks a lot for taking care and replacing an outdated third party package with stdlib means.
I have only one comment, although as stated I am not sure whether this is feasible and reasonable to include.

cobald_tests/daemon/config/test_python.py Show resolved Hide resolved
rcaspart
rcaspart previously approved these changes Mar 23, 2021
View reviewed changes
Copy link
Member

@rcaspart rcaspart left a comment

Choose a reason for hiding this comment

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

Thanks a lot, looks good to go to me 👍

eileen-kuehn
eileen-kuehn requested changes Mar 23, 2021
View reviewed changes
Copy link
Member

@eileen-kuehn eileen-kuehn left a comment

Choose a reason for hiding this comment

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

Awesome you recognised that the used library so far is deprecated and that now you only rely on standards.
I never worked with those yet and also did not check the implementations. So please have a look on my comments before merging.

src/cobald/daemon/config/python.py Outdated
such as ``.py`` or ``.pyc`` for a plaintext or bytecode python module.
"""
global _loaded
current_index = _loaded = _loaded + 1
Copy link
Member

Choose a reason for hiding this comment

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

What is the actual purpose of _loaded? Is it to create unique module_names or do you also want to have a kind of counter on how many configurations have been loaded? I am just curious as in case of an error the counter is not being decremented.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's just to have a unique number. You are right that this isn't obvious, I'll think of another name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the naming to indicate this is for identifying modules. Also using an itertools.count to get rid of the global (and possible race condition).

src/cobald/daemon/config/python.py Show resolved Hide resolved
cobald_tests/daemon/config/test_python.py Show resolved Hide resolved
@maxfischer2781
Copy link
Member Author

Changed/commented the code to make it a tad more maintainable, and added a test for what I assume are expected usage errors.

eileen-kuehn
eileen-kuehn approved these changes Mar 23, 2021
View reviewed changes
Copy link
Member

@eileen-kuehn eileen-kuehn left a comment

Choose a reason for hiding this comment

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

Nice extension to the already great contribution. Go for it 👍

rcaspart
rcaspart approved these changes Mar 23, 2021
View reviewed changes
Copy link
Member

@rcaspart rcaspart left a comment

Choose a reason for hiding this comment

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

Nothing to add. Looks good.

@maxfischer2781 maxfischer2781 merged commit 892bc51 into master Mar 23, 2021
@maxfischer2781 maxfischer2781 deleted the maintenance/drop-include branch March 23, 2021 13:08
@maxfischer2781 maxfischer2781 mentioned this pull request Sep 15, 2021
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rcaspart rcaspart rcaspart approved these changes

@eileen-kuehn eileen-kuehn eileen-kuehn approved these changes

@giffels giffels Awaiting requested review from giffels

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

Successfully merging this pull request may close these issues.

Drop dependency on include package
3 participants
@maxfischer2781 @rcaspart @eileen-kuehn