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

Classifications refactor #42

Merged
merged 13 commits into from
Aug 27, 2023
Merged

Classifications refactor #42

merged 13 commits into from
Aug 27, 2023

Conversation

jatkinson1000
Copy link
Owner

@jatkinson1000 jatkinson1000 commented Aug 20, 2023

Closes #39
Consider also tackling #24

Refactor excessively long classifications file into several separate ones for each type of classification (plus general utils file).

It would be nice for the code to continue functioning as it previously did with

import archeryutils.classifications as class_func

rather than having to import each type of classification (indoor, outdoor, etc.) separately.
I have made an effort to do this using the classifications module __init__.py but it's not quite working.
@LiamPattinson is there any chance you could cast an eye and advise me what is best to do?
This could also be related to issue #3

TODO:

  • Update examples notebook

@jatkinson1000 jatkinson1000 self-assigned this Aug 20, 2023
@jatkinson1000 jatkinson1000 added the documentation Improvements or additions to documentation label Aug 20, 2023
@LiamPattinson
Copy link
Contributor

Sorry, I'm not really sure what behaviour you're trying to retain. Please could you post example snippets of how you'd like things to work and how things have to work instead due to this PR?

@jatkinson1000
Copy link
Owner Author

Basically I want it to work the same as previously, so import archeryutils.classifications as class_funcs but because classifications.py has been broken up I now have to import each file separately, e.g. import archeryutils.classifications.agb_outdoor_classifications as class_funcs_out.

I tried to make it so that I could have all of the functions in these files wrapped up into classifications with the __init__.py but it doesn't seem to be working.

@LiamPattinson
Copy link
Contributor

I think I've figured it out. In your top-level __init__.py, you have:

from archeryutils.classifications import classifications

So when you import archeryutils.classifications, you're actually importing the module archeryutils.classifications.classifications, which is now an empty module. It should work if you change the top-level __init__.py to instead say:

import archeryutils.classifications as classifications

@jatkinson1000
Copy link
Owner Author

I think I've figured it out. Change the top-level __init__.py to instead say:

import archeryutils.classifications as classifications

M8 that's done it.
Thank you so much, pypack guru.

@jatkinson1000
Copy link
Owner Author

Adding classification tests so that this PR will also close #23
This is necessary here to ensure everything is working OK after a major refactor.

@jatkinson1000 jatkinson1000 linked an issue Aug 24, 2023 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Aug 27, 2023

Codecov Report

Merging #42 (2e8a857) into main (7839100) will increase coverage by 18.56%.
Report is 3 commits behind head on main.
The diff coverage is 97.14%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main      #42       +/-   ##
===========================================
+ Coverage   71.47%   90.03%   +18.56%     
===========================================
  Files          11       21       +10     
  Lines         936     1114      +178     
===========================================
+ Hits          669     1003      +334     
+ Misses        267      111      -156     
Files Changed Coverage Δ
...cheryutils/classifications/classification_utils.py 88.67% <88.67%> (ø)
...ils/classifications/agb_outdoor_classifications.py 95.86% <95.86%> (ø)
...tils/classifications/agb_indoor_classifications.py 97.01% <97.01%> (ø)
.../classifications/agb_old_indoor_classifications.py 97.91% <97.91%> (ø)
...utils/classifications/agb_field_classifications.py 98.50% <98.50%> (ø)
archeryutils/__init__.py 100.00% <100.00%> (ø)
archeryutils/classifications/__init__.py 100.00% <100.00%> (ø)
...cheryutils/classifications/tests/test_agb_field.py 100.00% <100.00%> (ø)
...heryutils/classifications/tests/test_agb_indoor.py 100.00% <100.00%> (ø)
...utils/classifications/tests/test_agb_old_indoor.py 100.00% <100.00%> (ø)
... and 2 more

@jatkinson1000 jatkinson1000 merged commit bbacf50 into main Aug 27, 2023
12 of 13 checks passed
@jatkinson1000 jatkinson1000 deleted the class-refactor branch November 19, 2023 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor classifications Tests needed for classifications
2 participants