-
Notifications
You must be signed in to change notification settings - Fork 3
RDPS implementation with additional extensions #114
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
83 commits
Select commit
Hold shift + click to select a range
d737a9d
fix: add small fixes to create Items out of RDPS and HRDPS data sets …
henriaidasso 6bde69e
feat: rdps extension added
henriaidasso 911cbe9
feat: rdps collection config added
henriaidasso b5f0de9
feat: rdps implementation added
henriaidasso 76b37a7
fix: export rdps implementation for cli access
henriaidasso 5e5541a
fix: clean unused code comments
henriaidasso 4d6aba0
fix: clean unused code
henriaidasso 3866246
fix: filename for item id updated
henriaidasso 138f0dd
fix: item validation added
henriaidasso cd5e801
fix: item validation added
henriaidasso eb86c50
fix: add properties to item
henriaidasso bfdd825
feat: hrdps ext. and impl. added
henriaidasso 49804f5
fix: integrate properties
henriaidasso 99868fc
fix: add providers handling
henriaidasso 24066be
feat: cf extension added
henriaidasso 90c30c1
fix: make providers optional
henriaidasso 9ea89d7
feat: file info extension added
henriaidasso fc8806b
feat: dataclasses-json library added
henriaidasso ad2bba9
fix: contacts extension added
henriaidasso 6c45ef2
fix: file info extension added
henriaidasso 730763c
fix clean cf extension code
henriaidasso 0ca5211
fix: make contacts optional
henriaidasso 2e6a69e
Merge branch 'master' into rdps
henriaidasso a8c299d
fix: cordex test file fix
henriaidasso ec29535
fix: get assets check service_type type
henriaidasso 933352f
Merge remote-tracking branch 'refs/remotes/origin/rdps' into rdps
henriaidasso 27e2a40
fix: get assets check service_type type
henriaidasso 92bc676
fix: contact data model removed
henriaidasso 57a6836
fix: cf extension added using subclasses
henriaidasso d8c7b97
fix: hrdps impl. updated using subclass
henriaidasso 2c207a7
fix: cf item extension get assets filter
henriaidasso c03b5cf
fix: remove dataclass-json dependency
henriaidasso 126aa4c
doc: readme updated
henriaidasso 40fda07
fix: file extension added using subclass
henriaidasso c968b28
fix: hrdps class doctstring updated
henriaidasso b75bb54
fix: contact rtype update
henriaidasso f22b369
fix: providers updated
henriaidasso 7e6a764
fix: collection info updated
henriaidasso c462ecd
fix: file helper updated
henriaidasso e38ad09
fix: collection links updated
ahenrij dd7e480
fix: warning fixed
ahenrij 4597fe9
fix: rdps tests updated
ahenrij 29dc7e2
fix: populators updated
ahenrij ea75a9b
doc: comments added
ahenrij 26d9135
fix: update link title
henriaidasso acaeea8
fix: add smaller size logo
ahenrij af3d1fe
fix: merged changes
ahenrij ffa1021
doc: readme updated
ahenrij 5b60a09
fix: rdps collection info updated
ahenrij af32a48
fix: update model_fields access
ahenrij e24834a
fix: service type check in get_assets func
ahenrij e442359
fix: from data return type
ahenrij 85f70f1
fix: reuse populator session in file helper
ahenrij faae3a1
fix: reuse populator session in file helper
ahenrij 24a546d
fix: rdps extension updated
ahenrij 0b10d2a
fix: collection level metadata updated
ahenrij 0443acd
fix: update .gitignore
ahenrij dfe1437
fix: remove blank lines
ahenrij 387baa7
fix: set file asset key in helper init
ahenrij 11029e4
fix: udpate default variable values
ahenrij f904c4c
fix: optional file size if asset key absent
ahenrij 01ac7da
fix: refactor helpers instantiation
ahenrij 43a5d0b
fix: merged
ahenrij 29b2890
doc: changes updated
ahenrij 293a37e
doc: readme updated
ahenrij 64e39de
fix: collection links updated
ahenrij 660265d
fix: collection contacts updated
ahenrij afbe940
fix: rdps tests updated to check added fields
ahenrij 64c33b6
fix: add crim as indexer in providers
ahenrij b73c216
fix: add crim as indexer in contacts only
ahenrij 5b668e9
fix: license links updated
ahenrij 84ab682
fix: dimensions and variables updated
ahenrij 1a0b8aa
fix: dimensions and variables updated
ahenrij 4366e6b
Merge branch 'master' into rdps
henriaidasso 227a7d5
fix: precommit run
ahenrij 30054ea
Merge branch 'master' into rdps
henriaidasso 0dadae7
fix: updated get assets methods for uniformity
ahenrij 49f0d4a
fix: cf iterate on values updated
ahenrij 185ddac
fix: unit str to match validation schema
ahenrij 8cdbc59
fix: return none for file content-length issues
ahenrij 3f033c2
fix: remove unused future imports and adds in thredds
ahenrij 1fea5bd
Merge branch 'master' into rdps
henriaidasso 07696c4
fix: changes version fix
ahenrij File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
fix: rdps extension updated
- Loading branch information
commit 24a546d302d2bd1985b6dca92ad07e965c63ad75
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fmigneault @huard what do you think of this suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it adds a level of abstraction and complexity that is not needed, since the
field = "cf"must still be indicated anyway.I think something like this would be easier to use:
Note that in this example, I assume
cf_helperis the instance created upstream by whichever operation that createsRDPSDataModel(). There is no need to restart from the class type/annotation each time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unclear to me how that works with the existing code.
Currently, the class method (also a model pre-validator) is used to initialize the helper in a corresponding named model field (e.g., assigning CFHelper to the model field data["cf"]). This facilitate retrieval of the helpers later in the parent class by inspecting model_fields that are instances of the Helper class.
Could you elaborate more on how your suggestion of class method matches the existing pattern of automatically finding helpers? I also assume you propose making helpers callable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simply renamed
cf: CFHelpertocf_helper: CFHelperto allowdef cf(...)without conflict. I expect the same auto-instantiate behavior to happen.Once the attribute
cf_helpergets instantiated, we could invoke it directly. Creating the instance from the annotation makes the previous instance creation useless.I propose adding a
__call__()definition to allowcls.cf_helper(...)directly.The alternative would be to have something like
cls.cf_helper.from_data(...)or any other equivalent@classmethoddefinition, but I think it is not that clean.Just wondering though. Did you manage to have the
CFHelperandFileHelperto be created with additional parameters (auth, verify, etc.)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, let's go with the callable approach.
Yes I did. I set the session into the FileHelper via the
set_sessionmethod below. I didn't want to put the session object as additional params to the staticfrom_data(...)currently used to instantiate data models since session is not required in all helpers. You can see related changes inadd_RDPS.py,rdps.py, and the FileHelper class infile.pyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think
sessioncould be set directly in the helper using an attribute defined in their class definitions? I think the code would be more generic if it uses attributes directly rather than introducing new methods. If possible (using__call__,from_data, or another approach), ideallysessionwould be populated by name directly bypydanticwithout manually setting it. I'm not sure if it is easily (or at all) doable, but if so, that would be the most generalized way to trickle down attributes.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In think I found a way using optional **kwargs in the constructors with a combination of static from_data in helper classes. Waiting on the catalog to be up again to validate it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!