pinkopy user-friendly updation in README.md file and base_session.py … #33
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
theherk
requested changes
Jul 10, 2024
There was a problem hiding this comment.
First, thank you for contributing. However, it is very difficult to sift through all the imposed personal preferences to see what actually changed. Please separate into commits that illustrate what is actually changing the execution and those that are trivial changes to formatting.
Also, please use imperative mood in your commits.
|
|
||
| except Exception as e: | ||
| print(f"An error occurred: {e}") | ||
|
|
There was a problem hiding this comment.
I don't think recommending catching the base Exception is a good idea in any case. I don't see a reason to store client_id as a separate variable. I can see no other functional changes in the README.
| use_cache (bool, optional): Use cache? Defaults to True. | ||
| cache_ttl (int, optional): Duration cache lives. Defaults to 1200 seconds. | ||
| cache_methods (list, optional): List of methods to cache. Defaults to an empty list. | ||
| token (str, optional): Auth token for headers. |
There was a problem hiding this comment.
I don't understand what is actually changing here, but okay.
| def __init__(self, service, user, pw, use_cache=True, cache_ttl=1200, | ||
| cache_methods=None, token=None): | ||
|
|
||
| def __init__(self, service, user, pw, use_cache=True, cache_ttl=1200, cache_methods=None, token=None): |
There was a problem hiding this comment.
This seems to just be enforcing a style preference. If that is true, I hope it is based on black or something universal.
| self.service = service | ||
| self.user = user | ||
| self.pw = pw | ||
| self.headers = { | ||
| 'Authtoken': token, | ||
| 'Accept': 'application/json', | ||
| 'Content-type': 'application/json' | ||
| 'Content-Type': 'application/json' |
| if not self.headers['Authtoken']: | ||
| self.get_token() | ||
| self.__use_cache = bool(use_cache) |
There was a problem hiding this comment.
The reason this used bool, was to handle truthy and non-truthy values like "false". Is that problematic?
| @@ -56,23 +57,23 @@ def __init__(self, service, user, pw, use_cache=True, cache_ttl=1200, | |||
| self.__enable_method_cache(method_name) | |||
|
|
|||
| def __enable_method_cache(self, method_name): | |||
| """Enable cache for a method. | |||
| """ | |||
| Enable cache for a method. | |||
There was a problem hiding this comment.
This was following the Google Styleguide.
| headers = headers if headers else self.headers | ||
| attempt = attempt or 1 | ||
| service = service or self.service | ||
| headers = headers or self.headers |
There was a problem hiding this comment.
I don't think it is an improvement to short circuit this way, and is generally more preferred to use the removed structure, so it seems like more preferences.
| # Token went bad, login again. | ||
| log.info('Commvault token logged out. Logging back in.') | ||
| # Delay is so I don't get into recursion trouble if I can't login right away. | ||
| raise ValueError(f'HTTP method {method} not supported') |
There was a problem hiding this comment.
Just another preference for f-string over format.
|
Also, this could already be used not as a context manager; this is mentioned in the documentation. Did that not work for you? And I see you're contributing similar ambiguous pull requests elsewhere? Are you just trying to pump some contrib numbers or are you actually using Commvault? If you're an actual user, and you can clarify what this change is really about, I do really appreciate your effort. |
|
Mamlesh:
Actually wanted to get more involved on open source contributions. I was
exploring about commvault SDK that's when i found your repo, i found it
really interesting thought i would contribute.
Thankyou for going through my changes, really appreciate it. If you need
any help done, please let me know. I would love to contribute.
…On Wed, 10 Jul 2024 at 19:04, Adam Sherwood ***@***.***> wrote:
***@***.**** requested changes on this pull request.
First, thank you for contributing. However, it is very difficult to sift
through all the imposed personal preferences to see what actually changed.
Please separate into commits that illustrate what is actually changing the
execution and those that are trivial changes to formatting.
Also, please use imperative mood
<https://git.kernel.org/pub/scm/git/git.git/tree/Documentation/SubmittingPatches?id=HEAD#n307>
in your commits.
------------------------------
In README.md
<#33 (comment)>:
> + client_id = '1234'
+ backup_jobs = commvault.jobs.get_jobs(client_id, job_filter="Backup")
+
+ subclient_id = '12345678'
+ recent_subclient_jobs = commvault.jobs.get_subclient_jobs(backup_jobs, subclient_id, last=3)
+
+ for job in recent_subclient_jobs:
+ job_id = job['jobSummary']['jobId']
+ job_details = commvault.jobs.get_job_details(client_id, job_id)
+ job_vmstatus = commvault.jobs.get_job_vmstatus(job_details)
+
+ print(f"Job ID: {job_id}, VM Status: {job_vmstatus}")
+
+except Exception as e:
+ print(f"An error occurred: {e}")
+
I don't think recommending catching the base Exception is a good idea in
any case. I don't see a reason to store client_id as a separate variable.
I can see no other functional changes in the README.
------------------------------
In pinkopy/base_session.py
<#33 (comment)>:
>
Args:
- service (optional[str]): URL and path to root of api
- user (str): Commvault username
- pw (str): Commvault password
- use_cache (optional[bool]): Use cache? Defaults to False
- cache_ttl (optional[int]): Duration cache lives. Defaults to 1200.
- cache_methods (optional[int]): List of methods to cache.
- Defaults provided by the inheriting classes.
- token (optional[str]): Authtoken for header
+ service (str): URL and path to root of API.
+ user (str): Commvault username.
+ pw (str): Commvault password.
+ use_cache (bool, optional): Use cache? Defaults to True.
+ cache_ttl (int, optional): Duration cache lives. Defaults to 1200 seconds.
+ cache_methods (list, optional): List of methods to cache. Defaults to an empty list.
+ token (str, optional): Auth token for headers.
I don't understand what is actually changing here, but okay.
------------------------------
In pinkopy/base_session.py
<#33 (comment)>:
> """
- def __init__(self, service, user, pw, use_cache=True, cache_ttl=1200,
- cache_methods=None, token=None):
+
+ def __init__(self, service, user, pw, use_cache=True, cache_ttl=1200, cache_methods=None, token=None):
This seems to just be enforcing a style preference. If that is true, I
hope it is based on black or something universal.
------------------------------
In pinkopy/base_session.py
<#33 (comment)>:
> self.service = service
self.user = user
self.pw = pw
self.headers = {
'Authtoken': token,
'Accept': 'application/json',
- 'Content-type': 'application/json'
+ 'Content-Type': 'application/json'
Good catch.
------------------------------
In pinkopy/base_session.py
<#33 (comment)>:
> if not self.headers['Authtoken']:
self.get_token()
- self.__use_cache = bool(use_cache)
The reason this used bool, was to handle truthy and non-truthy values like
"false". Is that problematic?
------------------------------
In pinkopy/base_session.py
<#33 (comment)>:
> @@ -56,23 +57,23 @@ def __init__(self, service, user, pw, use_cache=True, cache_ttl=1200,
self.__enable_method_cache(method_name)
def __enable_method_cache(self, method_name):
- """Enable cache for a method.
+ """
+ Enable cache for a method.
This was following the Google Styleguide
<https://github.com/google/styleguide/blob/gh-pages/pyguide.md#38-comments-and-docstrings>
.
------------------------------
In pinkopy/base_session.py
<#33 (comment)>:
> allowed_attempts = 3
- attempt = 1 if not attempt else attempt
- service = service if service else self.service
- headers = headers if headers else self.headers
+ attempt = attempt or 1
+ service = service or self.service
+ headers = headers or self.headers
I don't think it is an improvement to short circuit this way, and is
generally more preferred to use the removed structure, so it seems like
more preferences.
------------------------------
In pinkopy/base_session.py
<#33 (comment)>:
> url += '?' + urlencode(qstr_vals)
res = requests.get(url, headers=headers, params=payload)
elif method == 'PUT':
res = requests.put(url, headers=headers, json=payload)
elif method == 'DELETE':
res = requests.delete(url, headers=headers)
else:
- raise ValueError('HTTP method {} not supported'.format(method))
- if (res.status_code == 401
- and headers['Authtoken'] is not None
- and attempt <= allowed_attempts):
- # Token went bad, login again.
- log.info('Commvault token logged out. Logging back in.')
- # Delay is so I don't get into recursion trouble if I can't login right away.
+ raise ValueError(f'HTTP method {method} not supported')
Just another preference for f-string over format.
—
Reply to this email directly, view it on GitHub
<#33 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A27VX6CDXAH2SFNQQZSDNW3ZLUZ7XAVCNFSM6AAAAABKU3KX4KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNRZGA2DIOBYGU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Okay, so will you be updating the patch as requested? Can you explain what errors you had using the package as it is? Also, why did you start your reply with your own name like it is a message to yourself? Really just makes this feel like AI spam. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I updated the code to use pinkopy for interacting with Commvault's API more flexibly. Now, instead of using a context manager (with statement) every time, you can create a CommvaultSession instance directly. This allows you to manage the session manually and use the API methods directly on the session instance. Additionally, the code remains compatible with older usage patterns, so methods can still be called in the old way for familiarity and ease of transition.