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

Remove pkg resources #262

Merged
merged 27 commits into from
Jan 14, 2024
Merged

Remove pkg resources #262

merged 27 commits into from
Jan 14, 2024

Conversation

jaik03
Copy link
Contributor

@jaik03 jaik03 commented Jan 7, 2024

Updated plugins.py and tool.py to pkg_resources to use importlib.metadata instead. Fixed tests to reflect using entry_points call to populate list_plugins and get_plugins.

For reference on pkg_resources:
https://setuptools.pypa.io/en/latest/pkg_resources.html#package-discovery-and-resource-access-using-pkg-resources

@jaik03 jaik03 marked this pull request as draft January 7, 2024 17:54
@jaik03
Copy link
Contributor Author

jaik03 commented Jan 7, 2024

I neglected to run the "mypy src" check. I'm able to fix it but now I have to rectify it in the test_plugins as well.

@kedder
Copy link
Owner

kedder commented Jan 7, 2024

Hopefully we are not breaking existing plugins.

@jaik03
Copy link
Contributor Author

jaik03 commented Jan 12, 2024

I'm going to close this until I can get the build to work correctly. I have to install importmetadata-lib on python versions less than 3.10 and I can't seem to get the pip lockfile to work correctly.

@jaik03 jaik03 closed this Jan 12, 2024
@jaik03
Copy link
Contributor Author

jaik03 commented Jan 14, 2024

I tested this workflow on my GitHub and it was successfully building. Ready for review now.

Considering the nature of the changes, it shouldn't break other plugins as their interfacing through console scripts entry points. I've tested by loading several different plugins as well as the one I'm developing and it remains functional, though I don't have much ability to call the other plugins as I don't have test data. I could see it break a plugin if they were importing pkg_resources for use in their code without declaring as a dependency, but that's an easy fix. They should also probably update to move away from pkg_resources in any case.

You can also sit on this PR, the negative performance factors of pkg_resources probably aren't super impactful for most people's use case, but the library is deprecated so at some point it will have to be done. I mostly did this to explore the recent python packaging trends and get a feel for some of the development factors. If there are other ways you want to test or explore this before accepting the PR, let me know.

@jaik03 jaik03 reopened this Jan 14, 2024
@coveralls
Copy link

coveralls commented Jan 14, 2024

Coverage Status

coverage: 95.74% (+0.02%) from 95.724%
when pulling 653f990 on jaik03:Remove-pkg_resources
into 2e1414a on kedder:master.

@jaik03 jaik03 marked this pull request as ready for review January 14, 2024 02:43
@kedder
Copy link
Owner

kedder commented Jan 14, 2024

Thank you so much for this! This switch have been long overdue.

@kedder kedder merged commit b5f7861 into kedder:master Jan 14, 2024
11 checks passed
@jaik03
Copy link
Contributor Author

jaik03 commented Jan 14, 2024

NP. Thanks for creating and maintaining this. It's very helpful now that my bank eliminated direct connect OFX, and I'm enjoying exploring about the OFX standard and Python.

@kedder
Copy link
Owner

kedder commented Jan 14, 2024

Let me know when you want to cut a new version.

@jaik03
Copy link
Contributor Author

jaik03 commented Jan 14, 2024

I've got a few more changes to make. I'm working on one within ofx.py to enhance the security name information it returns when it's available as part of the statement that I'd prefer to work out first.

@kedder
Copy link
Owner

kedder commented Jan 14, 2024

sound good!

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.

4 participants