-
Notifications
You must be signed in to change notification settings - Fork 79
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
Add row to calculate coordinates of additional entity #312
Conversation
@@ -175,10 +171,22 @@ def append_select_one_from_file_row(df: pd.DataFrame, entity_name: str) -> pd.Da | |||
} | |||
) | |||
|
|||
# Prepare the row for calculating coordinates based on the additional entity | |||
coordinates_row = pd.DataFrame( |
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 quite nice having the field in code like this.
It's verbose, but we can then use source control.
Do you think we should remove the mandatory fields xls file and build the dataframe here instead?
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 we tried that initially when we were writing this function to append fields. But it looked so messy and complex so we decided to use a file instead, updating the form seemed easier during that 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.
Thanks for being my memory 😆
It probably did look messy! But we could clean it up a bit, either using functions or different files to split it up nicely.
I'm leaning more in favour of putting it all in code so it's source controlled and easily visible what we change.
Not high priority to do right now if it's going to take some time. But do you think it's a good idea in the long run? Or we just keep it simple with the file?
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's good approach to maintain it in the codebase rather than XLS file. But it still has both merits and demerits.
Appending all mandatory fields using code base will help us to track changes (use source control), it reduces the complexity of maintaining xlsx files, we can also write test cases to ensure dataframe construction and validation.
While for non-developers (project manager) who want to update the mandatory forms, it might be challenging for them. For small datasets, this isn’t an issue, but as the number of mandatory fields grows, managing them directly in the code might become troublesome. @manjitapandey
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.
That's a valid concern for sure, but honestly I can't see anyone modifying the mandatory fields form that isn't a developer 😅
We would have to think up a nice way to manage this without it being too verbose and cumbersome.
I'll merge this PR and we can consider this in the future. Not a high priority right now, as it's a bit of work 👍
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.
okay, I will work on this side by side. We can merge then for now.
Description:
This feature introduces functionality to calculate and retrieve the coordinates of an additional entity selected in a survey form.
Issue: