-
-
Notifications
You must be signed in to change notification settings - Fork 4
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
Issue/55 get sites details #63
Conversation
select how to get site details: site_uuid or client_site_id
test for select site id passing
src/sites_toolbox.py
Outdated
|
||
def get_site_details(session, site_selection): | ||
"""Get the site details for one site""" | ||
site_uuid = get_site_by_uuid(session=session, site_uuid=site_selection) |
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.
should this be site=get_site_by_uuid(session=session, site_uuid=site_selection)
src/sites_toolbox.py
Outdated
site_list= [str(site.client_site_id) for site in get_all_sites(session=dbsession)] | ||
selected_id = st.selectbox("Sites by client_site_id", site_list) | ||
selected_id = get_site_by_client_site_id(session=dbsession, client_site_id = selected_id) | ||
selected_id = str(selected_id.site_uuid) |
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.
add else:
raise ValueError, if query_method not in ....
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.
maybe selected_id
is actually selected_site_uuid
return site_details | ||
|
||
def select_site_id(dbsession, query_method): | ||
if query_method == "site_uuid": |
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.
could you add a function description to say what this does
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.
worth perhaps still adding a function descirption, but just do this in a seperate PR
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.
see comments, just a few changes, but looks good
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.
2 comments, but then it looks ready to go
Pull Request
Description
This PR adds the feature and tests for getting details for one site, showing metadata.
This is what the metadata looks like. I could use a little help extracting the names from the weird dictionary thing for
DNO
,GSP
, andRegion
(still needs to be added). And looking at it now, some of the metadata could use formatting.:I have added the option for selecting a site by
client_site_id
and added a function toget_data.py
toget_site_by_client_site_id
but need some help not getting an error when theclient_site_id
is not unique:It'll be easy to add the
client_site_name
search option to the existing function, but for now, since most of the these names areNone
, I wasn't convinced there was much point in adding this option just yet. Maybe it could be added in cases of "not None".Fixes #55
How Has This Been Tested?
Wrote pytests and ran the code locally.
test_get_site_details
is currently failing. When I removedsite_group_name
from the dictionary altogether, the test passed. I'm wondering how to get around this.Checklist: