Add musuems_victoria_fetch.py#216
Conversation
There was a problem hiding this comment.
Please remove these changes from the pull request (PR). Automation will be implemented after this PR is merged.
There was a problem hiding this comment.
@sbarhin it is unhelpful to mark a conversation resolved when the pull request (PR) hasn't been updated with a resolution. Please don't.
I don't see any added or updated tests. Why did you check this item?
I don't see any added or updated documentation. Why did you check this item? |
| except requests.exceptions.RequestException as e: | ||
| LOGGER.error(f"Error fetching page {current_page} for {record_type}: {e}") | ||
| break |
There was a problem hiding this comment.
Please model exception handling on existing scripts:
quantifying/scripts/1-fetch/github_fetch.py
Lines 152 to 157 in ddb3958
There was a problem hiding this comment.
@sbarhin it is unhelpful to mark a conversation resolved when the pull request (PR) hasn't been updated with a resolution. Please don't.
There was a problem hiding this comment.
I apologize. I will take note next time.
There was a problem hiding this comment.
Please be sure to run static analysis before committing
There was a problem hiding this comment.
Please in the pre-commit-config.yaml, can I update the python version to 3.13?
|
|
||
| def initialize_data_file(file_path, header): | ||
| if not os.path.isfile(file_path): | ||
| with open(file_path, "w", newline="") as file_obj: |
There was a problem hiding this comment.
Please update this per:
| 'MEDIA JSON': sanitize_string(media_json_string), | ||
| } | ||
| initialize_data_file(FILE_RECORDS, HEADER_RECORDS) | ||
| with open(FILE_RECORDS, "a", newline="") as file: |
There was a problem hiding this comment.
Please update this per:
There was a problem hiding this comment.
Please rename to match convention.
The script should be executable:
There was a problem hiding this comment.
@TimidRobot Hi there, please can you explain further on "renaming to match convention"?
There was a problem hiding this comment.
Look at how the fetch scripts are name in the main branch. Match that convention.
There was a problem hiding this comment.
Can I name it museums_fetch.py? I guess adding the victoria made it quite longer
|
@sbarhin Please pay better attention to the documentation, instructions, and the conventions set by the existing scripts. |
| "--enable-save", | ||
| action="store_true", | ||
| help="Enable saving results", | ||
| default="true" |
There was a problem hiding this comment.
As demonstrated in the existing scripts, this shouldn't default to true.
The scripts should run as safely as possible without any command line options.
There was a problem hiding this comment.
@sbarhin it is unhelpful to mark a conversation resolved when the pull request (PR) hasn't been updated with a resolution. Please don't.
| "--enable-git", | ||
| action="store_true", | ||
| help="Enable git actions (fetch, merge, add, commit, and push)", | ||
| default="true" |
There was a problem hiding this comment.
As demonstrated in the existing scripts, this shouldn't default to true.
The scripts should run as safely as possible without any command line options.
There was a problem hiding this comment.
@sbarhin it is unhelpful to mark a conversation resolved when the pull request (PR) hasn't been updated with a resolution. Please don't.
|
@sbarhin good work implementing their API |
|
@TimidRobot Please it seems this PR is closed. What could be the issue? |
|
@sbarhin It says you closed it? |
Oops. Could I have done that mistakenly? I was pushing the changes in relation to the reviews. I later found that it says I closed it |
|
@oree-xx @TimidRobot Please what's the way forward now |
|
@sbarhin You would need @TimidRobot to look into it. |
Okay, thank you. |
|
I had issues changing the git credentials. I will create a new PR |
This PR implements automation for Museum Victoria data fetching as discussed in issue #215. The implementation follows the established patterns from existing fetch scripts.
This purpose of this file is to fetch all the records from the Museum Victoria API, then saving the necessary response fields needed for the next phase (processing phase).
Fixes
Description
Checklist
Update index.md).mainormaster).visible errors.
Developer Certificate of Origin
For the purposes of this DCO, "license" is equivalent to "license or public domain dedication," and "open source license" is equivalent to "open content license or public domain dedication."
Developer Certificate of Origin