You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
**[Thought, non-blocking]** One high-level thought that I have for you about the design of the functions in this module: Currently, there are three possible filter conditions (no filters; all filters; or year _but not_ township filter) and each one of the `build_query`, `generate_groups`, and `upload` functions are responsible for implementing a different solution for each condition. This works fine, but it makes the functions hard to read since each one has its own set of complex conditional branches, and if we ever needed to alter those conditions or add a new one I think we would find ourselves having to basically rewrite the whole module to accommodate the new or changed condition.
I think there's potential for a simpler design that consolidates all of the responsibility for evaluating the filters in the generate_groups function, which could return a list of groups that could then be operated on by each of the build_query and upload functions agnostic of the exact contents of the filters themselves. This is a bit speculative, so I would be happy to take a stab at a follow-up PR to demonstrate a refactor if you're interested but not sure what I mean. Let me know and we can hash it out! But I don't think it's necessary to get too deep into it while you're still trying to just get the functionality up and running.
I think there's potential for a simpler design that consolidates all of the responsibility for evaluating the filters in the
generate_groups
function, which could return a list of groups that could then be operated on by each of thebuild_query
andupload
functions agnostic of the exact contents of the filters themselves. This is a bit speculative, so I would be happy to take a stab at a follow-up PR to demonstrate a refactor if you're interested but not sure what I mean. Let me know and we can hash it out! But I don't think it's necessary to get too deep into it while you're still trying to just get the functionality up and running.Originally posted by @jeancochrane in #566 (comment)
The text was updated successfully, but these errors were encountered: