Implemented convenience method for client id by Sharvin-M · Pull Request #97 · nasa/python_cmr

Sharvin-M · 2025-01-14T21:05:28Z

Wrote the method for a user to specify the 'Client-Id' header in the Query class. By default, the method should return the Client-Id of the request as follows: "Client-Id: python_cmr-v0.13.0". If a client-id parameter is specified by the user, it should return: "Client-Id": f"{client_id} python_cmr-v0.13."

This is my first time contributing to open source, so I will appreciate any and all feedback! @chuckwondo @frankinspace

frankinspace

Thank you for the contribution! I added a couple suggestions.

frankinspace · 2025-01-14T21:26:25Z

cmr/queries.py

+        """
+
+        if not client_id: 
+            self.headers.update({"Client-Id: python_cmr-v0.13.0"}) 


Suggested change

self.headers.update({"Client-Id: python_cmr-v0.13.0"})

self.headers.update({"Client-Id": "python_cmr-v0.13.0"})

frankinspace · 2025-01-14T21:27:21Z

cmr/queries.py


        return self

+    def client_id(self, client_id: str) -> Self:


Should be able to use https://docs.python.org/3/library/importlib.metadata.html#importlib.metadata.version to dynamically replace the version of python_cmr in use instead of hard-coding it in the string.

frankinspace · 2025-01-14T21:34:31Z

I'd also suggest adding a quick test function in https://github.com/nasa/python_cmr/blob/35b8bdafa655c9a95edc41659ccb867df9e86c18/tests/test_queries.py to test that the custom string is appended as expected to the header.

I'd also suggest calling the function from the __init__ function: https://github.com/nasa/python_cmr/blob/develop/cmr/queries.py#L61
So that the default value is added as a header when the object is first initialized. Any subsequent calls to client_id would override the initial value.

chuckwondo

@Sharvin-M, thanks for your first contribution!

chuckwondo · 2025-01-14T22:25:32Z

cmr/queries.py


        return self

+    def client_id(self, client_id: str) -> Self:


To avoid confusion, I suggest that you do not name the function parameter the same as the function itself. I suggest changing the function parameter name to id_ instead of client_id. Note the trailing underscore on id_ because there is a global function named id, so the trailing underscore is a convention used to avoid name collisions with global variables.

chuckwondo · 2025-01-14T22:26:19Z

cmr/queries.py


+    def client_id(self, client_id: str) -> Self:
+        """
+        Set the value of this query's 'Client ID' header according to User's input.


Suggested change

Set the value of this query's 'Client ID' header according to User's input.

Set the value of this query's `Client-Id` header.

chuckwondo · 2025-01-14T22:29:29Z

cmr/queries.py

+        Otherwise, set the specified paramter option to the value along with 
+        the suffix (python_cmr-vX.Y.Z) and a space character between the specified paramter and the suffix.


Suggested change

Otherwise, set the specified paramter option to the value along with

the suffix (python_cmr-vX.Y.Z) and a space character between the specified paramter and the suffix.

Otherwise, set the header value to the specified value along with

the suffix `(python_cmr-vX.Y.Z)`, separated by a space character.

Sharvin-M · 2025-01-15T02:52:00Z

I attempted to add the requested changes, let me know your thoughts! @frankinspace @chuckwondo

cmr/queries.py

chuckwondo · 2025-01-15T15:22:10Z

cmr/queries.py

+        if not id_: 
+            return self
+
+        self.headers.update(
+            {"Client-Id": f"{id_} python_cmr-v{version('python_cmr')}"}
+        )


Suggested change

if not id_:

return self

self.headers.update(

{"Client-Id": f"{id_} python_cmr-v{version('python_cmr')}"}

)

python_cmr_id = f"python_cmr-v{version('python_cmr')}"

self.headers["Client-Id"] = f"{id_} ({python_cmr_id})" if id_ else python_cmr_id

chuckwondo · 2025-01-15T15:22:36Z

cmr/queries.py

        self.mode(mode)
        self.concept_id_chars: Set[str] = set()
        self.headers: MutableMapping[str, str] = {}
+        self.headers.update({"Client-Id": f"python_cmr-v{version('python_cmr')}"})


Suggested change

self.headers.update({"Client-Id": f"python_cmr-v{version('python_cmr')}"})

self.client_id()

chuckwondo · 2025-01-15T15:24:11Z

tests/test_queries.py

+    query.token("token")
+
+    expected_version = version("python_cmr")
+    assert query.headers["Client-Id"] == f"test_client python_cmr-v{expected_version}"


Suggested change

assert query.headers["Client-Id"] == f"test_client python_cmr-v{expected_version}"

assert query.headers["Client-Id"] == f"test_client (python_cmr-v{expected_version})"

frankinspace · 2025-01-15T15:34:11Z

For some reason the VCR tests in multiple queries are failing. I would have expected failure due to unexpected new header but they seem to be failing in a way that is causing twice the number of requests to be sent

chuckwondo · 2025-01-15T16:29:33Z

For some reason the VCR tests in multiple queries are failing. I would have expected failure due to unexpected new header but they seem to be failing in a way that is causing twice the number of requests to be sent

I can dig into it, if you like.

Co-authored-by: Chuck Daniels <cjdaniels4@gmail.com>

Sharvin-M · 2025-01-17T17:49:28Z

I looked into it a bit, Please let me know if any of this is useful info @frankinspace @chuckwondo

1. There was a type error because no arg is provided to the client_id method in the init function. To temporarily bypass this, I passed in the default argument manually (on line 72). I don't entirely understand how optional arguments work in Python.

# line 72
self.client_id(f"python_cmr-v{version('python_cmr')}")

2. I was then able to run the tests, which had one failing related to the get_all method:

3. There were many deprecation warnings related to not using the results method in all of the tests being run.

cmr/queries.py

Co-authored-by: Chuck Daniels <cjdaniels4@gmail.com>

added client id method

c720dcc

Sharvin-M marked this pull request as draft January 14, 2025 21:06

Sharvin-M marked this pull request as ready for review January 14, 2025 21:06

frankinspace suggested changes Jan 14, 2025

View reviewed changes

frankinspace linked an issue Jan 14, 2025 that may be closed by this pull request

Provide convenience method for adding a client-id header #49

Open

chuckwondo requested changes Jan 14, 2025

View reviewed changes

Sharvin-M added 2 commits January 14, 2025 18:47

updated client_id method

9f9d247

updated tests for client_id method

d92aa64

Sharvin-M requested review from chuckwondo and frankinspace January 15, 2025 02:50

frankinspace reviewed Jan 15, 2025

View reviewed changes

cmr/queries.py Outdated Show resolved Hide resolved

Update cmr/queries.py

10ec673

chuckwondo requested changes Jan 15, 2025

View reviewed changes

Sharvin-M and others added 2 commits January 15, 2025 09:21

Update cmr/queries.py

12f11b1

Co-authored-by: Chuck Daniels <cjdaniels4@gmail.com>

added requested changes

132ab4e

chuckwondo reviewed Jan 17, 2025

View reviewed changes

cmr/queries.py Outdated Show resolved Hide resolved

Update cmr/queries.py

38e723b

Co-authored-by: Chuck Daniels <cjdaniels4@gmail.com>

	self.headers.update({"Client-Id: python_cmr-v0.13.0"})
	self.headers.update({"Client-Id": "python_cmr-v0.13.0"})

	Set the value of this query's 'Client ID' header according to User's input.
	Set the value of this query's `Client-Id` header.

		Otherwise, set the specified paramter option to the value along with
		the suffix (python_cmr-vX.Y.Z) and a space character between the specified paramter and the suffix.

	self.headers.update({"Client-Id": f"python_cmr-v{version('python_cmr')}"})
	self.client_id()

	assert query.headers["Client-Id"] == f"test_client python_cmr-v{expected_version}"
	assert query.headers["Client-Id"] == f"test_client (python_cmr-v{expected_version})"

Conversation

Sharvin-M commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frankinspace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frankinspace commented Jan 14, 2025

Uh oh!

chuckwondo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sharvin-M commented Jan 15, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frankinspace commented Jan 15, 2025

Uh oh!

chuckwondo commented Jan 15, 2025

Uh oh!

Sharvin-M commented Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. There was a type error because no arg is provided to the client_id method in the init function. To temporarily bypass this, I passed in the default argument manually (on line 72). I don't entirely understand how optional arguments work in Python.

2. I was then able to run the tests, which had one failing related to the get_all method:

3. There were many deprecation warnings related to not using the results method in all of the tests being run.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Sharvin-M commented Jan 14, 2025 •

edited

Loading

Sharvin-M commented Jan 17, 2025 •

edited

Loading