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

make replace parameter work in edit_dataset_metadata #146

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

Conversation

pallinger
Copy link

@pallinger pallinger commented Jun 16, 2022

  • do not delete params in put_request
  • fix replace documentation and parametrization in edit_dataset_metadata

The main problem was that params was overwritten by {} in put_request at the beginning, effectively deleting any parameters passed. This caused the replace parameter in edit_dataset_metadata to not work.
There was also some confusion with is_replace in the documentation of edit_dataset_metadata, and also passing boolean instead of string "true" (although that may work, this is more in-line with https://guides.dataverse.org/en/latest/api/native-api.html#edit-dataset-metadata)

- fix replace documentation and parametrization in edit_dataset_metadata
@pallinger
Copy link
Author

Could someone review this?

@skasberger
Copy link
Member

Update: I left AUSSDA, so my funding for pyDataverse development has stopped.

I want to get some basic funding to implement the most urgent updates (PRs, Bug fixes, maintenance work). If you can support this, please reach out to me. (www.stefankasberger.at). If you have feature requests, the same.

Another option would be, that someone else helps with the development and / or maintenance. For this, also get in touch with me (or comment here).

@pdurbin
Copy link
Member

pdurbin commented Mar 4, 2023

@pallinger hi! Is this a fix for the following issue?

@pallinger
Copy link
Author

@pdurbin Yes, it seems to be.

@pdurbin
Copy link
Member

pdurbin commented May 18, 2023

Ok, thanks, I added "closes" to the description.

Copy link

sonarcloud bot commented Dec 4, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Collaborator

@shoeffner shoeffner left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR!
The documentation strings are a good catch and should definitely be adjusted, however, I disagree with some of the code changes and added a few comments why.

@@ -185,7 +185,7 @@ def post_request(self, url, data=None, auth=False, params=None, files=None):
"ERROR: POST - Could not establish connection to API: {0}".format(url)
)

def put_request(self, url, data=None, auth=False, params=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should stay None.
It is almost always better to use None instead of {} or [], the common pattern being:

def fun(arg=None):
    if arg is None:
        arg = {}

The reason is, roughly speaking, that python will process the def-line at its first pass through the code, initializing all arguments given there once and storing the references.
If the underlying argument would then, e.g., be appended to, this would modify the same object for each call. Consider:

def fun(arg=[]):
    arg.append(1)
    print(arg)
fun()
fun()
fun()

This would print:

[1]
[1, 1]
[1, 1, 1]

However, if the expected output is [1], [1], [1], the code should be:

def fun(arg=None):
    if arg is None:
        arg = []
    arg.append(1)
    print(arg)
fun()
fun()
fun()

While it is not so important here, I would still recommend sticking with the convention unless there's good reason not to.

For more information, see https://docs.python.org/3/faq/programming.html#why-are-default-values-shared-between-objects.

@@ -1288,7 +1287,7 @@ def edit_dataset_metadata(
url = "{0}/datasets/editMetadata/{1}".format(
self.base_url_api_native, identifier
)
params = {"replace": True} if replace else {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can stay as it is. Maybe with requests this was necessary, but with httpx I tried this:

>>> import httpx
>>> httpx.get('https://example.org', params={'bool': True, 'string': 'true'}).url
URL('https://example.org?bool=true&string=true')

So both seem to result in the same outcome. One could even consider using params = {"replace": replace} instead of the ternary. But I am not sure if the API supports it, so I would keep it as-is.

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.

edit_dataset_metadata replace bug
4 participants