Skip to content

Organise json files, cache loading.#160

Open
TomHall2020 wants to merge 3 commits into
jatkinson1000:mainfrom
TomHall2020:read_json
Open

Organise json files, cache loading.#160
TomHall2020 wants to merge 3 commits into
jatkinson1000:mainfrom
TomHall2020:read_json

Conversation

@TomHall2020

Copy link
Copy Markdown
Contributor

Small set of changes that would close #68 - does moving the json files cause any problems for your other systems @jatkinson1000 ? Was finding it tricky that they mixed in with the python files when sorted alphabetically.

Originally had thought of loading the data into global variables/constants - not sure if that actually helps anything compared to this approach of just caching the loading function - in all cases the behaviour is identical as the package loads and uses all the data at import time.

Reluctant to do any more on this as I feel the entire classification module structure needs an overhaul - too many modules with too many similar long names, but that is inevitable anyway for any refactoring to make a generic function ala #24 or #103

Bonus extras in the second two commits if you want it. One to clean up reading of the classes files - since we're min version 3.10 now can use structural pattern matching syntax to replace the if-else statements for selecting the class files. And one to remove the unused filepath parameter from the age/bowstyle/gender files - its all used at import time, and we are assuming the contents of the json files are correct in both the code and the type hints so there isn't really much purpose to load an alternative file at runtime - thats already not an option for the indoor/outdoor classes files, and it simplifies the signature.

…ata file reading

Removes duplicated definition of outdoor classes filename
Allows use of joinpath and clearer sequence of logic to early exit if type check fails on data rather than early return and defaulting to raising error.

@jatkinson1000 jatkinson1000 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks @TomHall2020

Happy with all the proposals here.
Agree this is neater, and the caching is a nice touch.

Happy with removing the optional file in the call signatures - this was in the old code and as you say it has evolved to a point where no other file would be passed in. I think it is nice retaining the data as a json file as it's easy to read and edit.

Any reason to use the json.loads(file.read_text()) over json.load(file) on an open file context?

Happy to overlook the failing coverage patch as those lines were not covered in the previous code.

Please could you drop something in the changelog to summarise.

@jatkinson1000

Copy link
Copy Markdown
Owner

@TomHall2020 a quick bump on this whilst I am active working on the 2026 Classification changes.

@codecov

codecov Bot commented Feb 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 62.85714% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 98.02%. Comparing base (70dd181) to head (678fdfc).
⚠️ Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
...cheryutils/classifications/classification_utils.py 62.85% 13 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #160      +/-   ##
==========================================
- Coverage   98.06%   98.02%   -0.05%     
==========================================
  Files          33       33              
  Lines        2170     2175       +5     
==========================================
+ Hits         2128     2132       +4     
- Misses         42       43       +1     
Files with missing lines Coverage Δ
...cheryutils/classifications/classification_utils.py 78.57% <62.85%> (+0.10%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jatkinson1000 jatkinson1000 force-pushed the main branch 4 times, most recently from 61111ce to 81bcce5 Compare May 28, 2026 10:48
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.

Data from read_json functions could be stored as constants

2 participants