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

Cleanup models #632

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Cleanup models #632

wants to merge 6 commits into from

Conversation

loganbyers
Copy link
Member

Suggesting some cleanup around the models and enums...

Replace sqlalchemy with vanilla enum

Pretty confident this will be without issue based on the way I have seen it used in the backend codebase... Really used as a way to get a limited set of strings... But worth pointing out that while the sqlalchemy Enum has typing similarity to enum.Enum they also have differences...

Use relative imports everywhere

Across the entire app/models/ directory there are inconsistent ways of performing imports sometimes looking at relative paths and othertimes looking from the root of the app. Under the assumption that there is no benefit/feature being utilized when performing an "absolute import" all imports have been reshaped to be relative imports.

Remove PGType from creation_options enum file

There were two instances of PGType - both of type Enum but defined in different files. The one that is removed seems redundant or dead... There was a slight difference in the set of enum options but based on my searching of the codebase it is fine to remove the one under the creation_options.py file.

danscales and others added 6 commits January 28, 2025 10:31
Merge develop to master (GTC-3125 changes)
Merge up (develop -> master) changes including admin id lookup, geostore proxy endpoints
Merge up fix for regions/subregions
modified:   app/models/enum/versions.py

Not known why this relied on sqlalchemy instead of the vanilla enum
module.
This bring this file into conformance with the rest of the enums.
Imports from within the project have been non-standard.
This standardizes them to always use relative imports.

modified:   app/models/enum/assets.py
modified:   app/models/orm/api_keys.py
modified:   app/models/pydantic/authentication.py
modified:   app/models/pydantic/downloads.py
modified:   app/models/pydantic/extent.py
modified:   app/models/pydantic/political.py
modified:   app/models/pydantic/query.py
modified:   app/models/pydantic/sources.py
modified:   app/models/pydantic/statistics.py
modified:   app/models/pydantic/symbology.py
There have been two PGType classes in `app/models` directory.
One came from the `app/models/enum/creation_options.py` file.
The other came from the `app/models/enum/pg_types.py` file.

The "creation option" PGType does not appear to be used anywhere within
the code and is probably some dead code from a refactor...
This enum has two fewer Postgres types than the other -- missing the
"timestamp_wtz" and "ARRAY" types...

This commit removes the "creation option" PGType while keeping the
"pg_types" PGType enum in place.

modified:   app/models/enum/creation_options.py
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 95.23810% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.87%. Comparing base (3c58c8f) to head (f86d2ea).

Files with missing lines Patch % Lines
app/models/pydantic/sources.py 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #632      +/-   ##
===========================================
- Coverage    79.92%   79.87%   -0.06%     
===========================================
  Files          133      133              
  Lines         6133     6117      -16     
===========================================
- Hits          4902     4886      -16     
  Misses        1231     1231              
Flag Coverage Δ
unittests 79.87% <95.23%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@dmannarino dmannarino left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for doing it!

@loganbyers
Copy link
Member Author

loganbyers commented Feb 13, 2025

Is there any reason why AssetBase.tags is a string and not a list of strings (link)?

Contrast it to DatasetMetadata which has Optional[List[str]]. There doesn't seem to be any tagging mechanism for VersionMetadata that I saw.

Most (every?) asset type's metadata inherits from AssetBase so I expect it would be a big ask to consider changing this...

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