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

updated use of entry_points according to importlib version >3.6.0 #11417

Merged
merged 4 commits into from
Oct 18, 2023
Merged

updated use of entry_points according to importlib version >3.6.0 #11417

merged 4 commits into from
Oct 18, 2023

Conversation

im-NL
Copy link
Contributor

@im-NL im-NL commented Oct 16, 2023

Summary

#11316 task 5.

  • updated use of entry_points according to importlib version >3.6.0
  • upgraded to latest version and ran the "kolibri plugin list" command in cli which uses the function changed and everything worked smoothly

Reviewer guidance

  • This change would throw an error if someone set up the repository without adding the --upgrade flag while installing the libraries. I have not changed the library version in requirements/base.txt either. Requesting further guidance on what to do.

Testing checklist

  • Contributor has fully tested the PR manually

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... SIZE: very small labels Oct 16, 2023
rtibbles
rtibbles requested changes Oct 16, 2023
View reviewed changes
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Just one tweak to make it backwards compatible!

kolibri/plugins/utils/__init__.py Outdated
@@ -481,7 +481,7 @@ def check_plugin_config_file_location(version):
def iterate_plugins():
# Use to dedupe plugins
plugin_ids = set()
for entry_point in entry_points().get("kolibri.plugins", []):
for entry_point in entry_points().select(group="kolibri.plugins"):
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, the task was insufficiently clear here - this needs to be done in a way that detects whether the output of entry points has a select method - in that case select should be used, otherwise the existing get should be used (this is why you are seeing test failures on earlier versions of Python).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct as usual. I apologise my thoughts are very constrained for now when it comes to contributing to big projects please bear with me 😅

@github-actions
Copy link
Contributor

github-actions bot commented Oct 16, 2023 •
edited
Loading

Build Artifacts

Asset type Download link
PEX file kolibri-0.16.0b6.dev0_git.216.g7ba08242.pex
Windows Installer (EXE) kolibri-0.16.0b6.dev0+git.216.g7ba08242-unsigned.exe
Debian Package kolibri_0.16.0b6.dev0+git.216.g7ba08242-0ubuntu1_all.deb
Mac Installer (DMG) kolibri-0.16.0b6.dev0+git.216.g7ba08242-0.3.0.dmg
Android Package (APK) kolibri-0.16.0b6.dev0+git.216.g7ba08242-0.1.0-debug.apk
TAR file kolibri-0.16.0b6.dev0+git.216.g7ba08242.tar.gz
WHL file kolibri-0.16.0b6.dev0+git.216.g7ba08242-py2.py3-none-any.whl

@im-NL
Copy link
Contributor Author

im-NL commented Oct 16, 2023

The python unit tests seem like they're failing for a different reason

I tested locally with the old version (2.1.1) and the new version (6.8.0) and both worked

@im-NL im-NL requested a review from rtibbles October 16, 2023 08:33
@rtibbles
Copy link
Member

Yes, these test failures are not because of the changes here, but rather because of tests that are not properly mocking calls to a remote server.

@im-NL @rtibbles
Update __init__.py
0151fb1 
Co-authored-by: Richard Tibbles <[email protected]>
@im-NL
Copy link
Contributor Author

im-NL commented Oct 16, 2023

Yes, thank you for pointing it out. Committed the changes :)
Will work on being more efficient from the get-go next time

@rtibbles
Copy link
Member

There's one last piece of feedback from the linter:

kolibri/plugins/utils/__init__.py:487:12: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`

@im-NL im-NL requested a review from rtibbles October 18, 2023 06:19
@rtibbles rtibbles merged commit 8450b14 into learningequality:develop Oct 18, 2023
34 checks passed
@rtibbles
Copy link
Member

Thanks for your work here @im-NL!

@rtibbles rtibbles mentioned this pull request Oct 18, 2023
Closed
AllanOXDi pushed a commit to AllanOXDi/kolibri that referenced this pull request Nov 3, 2023
@im-NL @rtibbles @AllanOXDi
updated use of entry_points according to importlib version >3.6.0 (le…
4959cc6 
…arningequality#11417)

* updated use of entry_points according to importlib version >3.6.0

* added backwards compatibility for importlib

* Update __init__.py

Co-authored-by: Richard Tibbles <[email protected]>

* use isInstance() for type

---------

Co-authored-by: Richard Tibbles <[email protected]>
poju3185 pushed a commit to poju3185/kolibri that referenced this pull request Mar 15, 2024
@im-NL @rtibbles @poju3185
updated use of entry_points according to importlib version >3.6.0 (le…
1ae8ccc 
…arningequality#11417)

* updated use of entry_points according to importlib version >3.6.0

* added backwards compatibility for importlib

* Update __init__.py

Co-authored-by: Richard Tibbles <[email protected]>

* use isInstance() for type

---------

Co-authored-by: Richard Tibbles <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rtibbles rtibbles rtibbles approved these changes

Assignees
No one assigned
Labels
DEV: backend Python, databases, networking, filesystem... SIZE: very small
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@im-NL @rtibbles