Skip to content
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

Dataset Province of Noord-Holland update to 2022 #542

Merged
merged 6 commits into from
Oct 28, 2024

Conversation

kaskranenburgQ
Copy link
Contributor

@kaskranenburgQ kaskranenburgQ commented Oct 7, 2024

This PR updates the province of Noord-Holland to 2022.
In this update we mostly took the 2019 dataset and updated the most notable changes in Klimaatmonitor. This effort is therefore useful for new studies with this dataset.

Goes together with: quintel/etsource#3135

Copy link
Member

@noracato noracato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @kaskranenburgQ! I understand your wish for a more clear message and value your enthusiasm, but changes like these should come in a separate PR. Right now I don't have the time or insight to review your changes, and that is blocking the merge of the new data.

What would work best next time is to open an issue where we can discuss the actual need (maybe more messages can be clarified) and update the code in all places necessary when we address it in the sprint.

If you could remove the code in the importable_commit and move it to an issue, that would be great!

@kaskranenburgQ
Copy link
Contributor Author

@noracato Alright! Will do that.

@kaskranenburgQ
Copy link
Contributor Author

With the removal of the changes for the file importable_commit.rb the migration will no longer run.
The error message as described in #545 pops up.
When I run the migration with the changes in importable_commit, it stated that the columns 'country' and 'name' where not in the edtiable attributes of the dataset and so it skipped over these keys . One would think that leaving them out of the migration with the regular code for importable_commit.rb would then be the solution. If you do an error pops up stating that 'country' and 'name' are missing mandatory headers, so the migration does not run. Therefore I'm not sure what to do anymore. @noracato From this description, do you have an idea about what's going on.

@kaskranenburgQ
Copy link
Contributor Author

Thanks to @louispt1 the issue is fixed! We can go ahead with the merge!

@kaskranenburgQ kaskranenburgQ merged commit f587c60 into production Oct 28, 2024
1 check passed
@kaskranenburgQ kaskranenburgQ deleted the data-update-pnh branch October 28, 2024 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants