Conversation
Nospamas
force-pushed
the
network_keys
branch
from
3d397f0 to
fc3f215
Compare
jameshiebert
left a comment
jameshiebert
left a comment
There was a problem hiding this comment.
This has an impact on my review of your PyCDS PR... it looks like most of the actual changes on this are renaming network_name to network_key, which is good in one respect in that it means that our invocations will remain static, even if we change the underlying network names. But...
This code essentially sits in between the invocations and the database layer, and the application layer (e.g. station-data-portal-XX and the PDP) that expose the data to our users. Are there any potential problems with how network_name is used in those downstream products? If it's just being used as a label then that's fine, but if it's utilized as a key (hopefully not) then we may have additional considerations before we flip the switch.
One thought that I have that's not aligned with this PR, is maybe we could instead add a "Network Display Name" column, rather than a network_key column. network_name has been historically utilized as a key, so we could continue to do so with minimal disruption. But we could modify the frontend display to use the new Network Display Name column that we can modify with impunity. The station-data-portal-frontend's metadata column already has short_name/long_name format, so we could just rewire that to use the display name.
Sorry, this is a bit of a curveball, but I think that it might still meet the requirement with less impact.
There was a problem hiding this comment.
I think this whole file precludes the existence of version control. It's maybe, kinda useful, but definitely not up-to-date with best practices. Worthy of removal rather than selective updates.
|
|
||
| def normalize(file_stream): | ||
| yield from normalize_swob(file_stream, "FLNRO-WMB", station_id_attr="stn_id") | ||
| yield from normalize_swob(file_stream, "flnro_wmb", station_id_attr="stn_id") |
There was a problem hiding this comment.
If we're updating keys, maybe we should fast forward them to present day. Not sure what's in the current spreadsheet, but probably something like "wlrs_wmb" or some reference to "wlrs" [pronounced Walrus].
|
|
||
| setup_logging( | ||
| "/home/james/code/git/crmprtd/logging.yaml", | ||
| "/home/james/code/git/crmprtd/logging.yaml", # TODO: remove references to specific installs |
There was a problem hiding this comment.
How about we do that now?
You're right. I'll throw together some alternative PRs and add a display name column. The issue arises primarily when our code has hard coded values for these names in conjunction with using a non-immutable column. So the two approaches become either:
Currently I'm opting for the latter, but the former might be preferable as you say. If adding a display column the main updates will need to be anywhere where the column is displayed. Off the top of my head this will primarily need to be the station data portal, though I'm not sure if it is exclusively that. In this PR's case we'll need to update any hard coded references to the network. I believe this is mostly crmprtd, though I do see a few other references using search such as here. I'll throw together display name PRs and we can have a think about which might be the least impactful. I think your longer history in PCIC makes you a better judge for this than me :) |
2 participants
This PR is complimentary to pacificclimate/pycds#250 on pycds and will update the package reference to a correct version once that is released.
This PR updates crmprtd to use the new network_key column available in pycds. This column is intended to be a simplified and unchanging human readable key that can be used to reference specific networks. This will replace the current references that use network_name which we intend to change as part of an upcoming data ingest.
It makes the following primary changes: