Skip to content
This repository has been archived by the owner on Aug 4, 2023. It is now read-only.

Remove mutable parameters in provider api scripts #100

Merged
merged 8 commits into from
Jun 23, 2021

Conversation

obulat
Copy link
Contributor

@obulat obulat commented Jun 9, 2021

Fixes WordPress/openverse#1732

This PR replaces mutable default parameters in provider api scripts.
This will eliminate one type of possible errors: when the default parameters change between the calls.

I split PR #67 into several PRs to make them more manageable and make reviews easier.

Signed-off-by: Olga Bulat obulat@gmail.com

Signed-off-by: Olga Bulat <obulat@gmail.com>
@obulat obulat requested a review from a team as a code owner June 9, 2021 09:01
@obulat obulat requested review from zackkrida and krysal June 9, 2021 09:01
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
To ensure that the tag list is immutable, a set was used. However, order in a set is not guaranteed, so the test fails because the order of extracted tags is incorrect. Set was replaced with tuple that does guarantee the order of elements.

Signed-off-by: Olga Bulat <obulat@gmail.com>
@obulat obulat mentioned this pull request Jun 9, 2021
Copy link
Member

@krysal krysal left a comment

Choose a reason for hiding this comment

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

Splitting the PRs is appreciated. I was thinking it would be good to also add .copy() when assigning constant dictionaries to prevent mutations completely. What do you think?

Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
krysal
krysal previously approved these changes Jun 11, 2021
Copy link
Member

@krysal krysal left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@krysal krysal mentioned this pull request Jun 14, 2021
zackkrida
zackkrida previously approved these changes Jun 21, 2021
Copy link
Member

@zackkrida zackkrida left a comment

Choose a reason for hiding this comment

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

Nice

# Conflicts:
#	src/cc_catalog_airflow/dags/provider_api_scripts/brooklyn_museum.py
#	src/cc_catalog_airflow/dags/provider_api_scripts/museum_victoria.py
#	src/cc_catalog_airflow/dags/provider_api_scripts/nypl.py
#	src/cc_catalog_airflow/dags/provider_api_scripts/science_museum.py
#	src/cc_catalog_airflow/dags/provider_api_scripts/smithsonian.py
#	src/cc_catalog_airflow/dags/provider_api_scripts/staten_museum.py
#	src/cc_catalog_airflow/dags/provider_api_scripts/wikimedia_commons.py
@obulat obulat dismissed stale reviews from zackkrida and krysal via 49333e5 June 23, 2021 05:37
@obulat obulat merged commit 5351b5e into main Jun 23, 2021
@obulat obulat deleted the replace_mutable_query_params branch June 23, 2021 05:41
@zackkrida zackkrida added the ✨ goal: improvement Improvement to an existing user-facing feature label Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
✨ goal: improvement Improvement to an existing user-facing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Quality] Remove mutable defaults in provider API scripts
3 participants