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

Miljoeportal #302

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open

Miljoeportal #302

wants to merge 34 commits into from

Conversation

kris927b
Copy link
Collaborator

@kris927b kris927b commented Dec 16, 2024

Adding the Danmarks Miljøportal data. #301

Merging this will also close #299 and #298

@kris927b kris927b self-assigned this Dec 16, 2024
This was linked to issues Dec 16, 2024
Copy link
Contributor

@KennethEnevoldsen KennethEnevoldsen left a comment

Choose a reason for hiding this comment

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

Code generally looks reasonable.

Though it is hard to review without the data.

However I believe we are missing metadata on these datasets?

To pull data run:

```bash
$ uv run data-processing/scripts/miljoeportal/download.py $PATH_TO_SAVE_RAW_DATA $PATH_TO_SAVE_EXTRACTED_DATA4
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$ uv run data-processing/scripts/miljoeportal/download.py $PATH_TO_SAVE_RAW_DATA $PATH_TO_SAVE_EXTRACTED_DATA4
$ python data-processing/scripts/miljoeportal/download.py $PATH_TO_SAVE_RAW_DATA $PATH_TO_SAVE_EXTRACTED_DATA4

I think we should be consistent throughout (I am perfectly fine with consistently using uv

Comment on lines +25 to +27
Returns:
bool: If the pull was succesful or not.
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Returns:
bool: If the pull was succesful or not.
"""
Returns:
If the pull was succesful or not.
"""

Copy link
Contributor

Choose a reason for hiding this comment

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

(generally if you already have the type hint I would not add it in docs

Comment on lines +59 to +60
Returns:
list[str]: _description_
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Returns:
list[str]: _description_


Args:
file_path (Path): Path to where to save raw files
id_ (str): Assess
Copy link
Contributor

Choose a reason for hiding this comment

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

?

@KennethEnevoldsen KennethEnevoldsen mentioned this pull request Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add data: Danmarks Miljøportal Add data from Cellar
2 participants