Fixes and improvements #864
Conversation
|
I reuse someone else's comment that lead to updates: Some feedback so we can integrate, or discuss, on this ? |
jenkinsapi/jenkins.py
Outdated
| not hasattr(self, "_get_plugins") | ||
| or self._get_plugins.baseurl != url | ||
| ): | ||
| self._get_plugins = Plugins(url, self) |
There was a problem hiding this comment.
I'd prefer something like self._plugin_cache to make it clear that it is data and not a method.
jenkinsapi/plugins.py
Outdated
| update_center = ( | ||
| "https://updates.jenkins.io/update-center.actual.json" | ||
| ) | ||
| jsonp = requests.get(update_center).content.decode("utf-8") |
There was a problem hiding this comment.
Why not directly use the json() method on the response?
There was a problem hiding this comment.
When I wrote this the first time, I didn't know about this possibility.
| @@ -37,7 +38,7 @@ def __eq__(self, other) -> bool: | |||
| return self.__dict__ == other.__dict__ | |||
|
|
|||
| def __str__(self) -> str: | |||
| return self.shortName | |||
| return f"{self.shortName}@{self.version}" | |||
There was a problem hiding this comment.
This change is causing a test failure
There was a problem hiding this comment.
=========================== short test summary info ============================
FAILED jenkinsapi_tests/unittests/test_plugins.py::TestPlugins::test_plugin_repr - AssertionError: '<jenkinsapi.plugin.Plugin subversion@Unknown>' != '<jenkinsapi.plugin.Plugin subversion>'
…gin should be downloaded from
for more information, see https://pre-commit.ci
| @@ -37,7 +38,7 @@ def __eq__(self, other) -> bool: | |||
| return self.__dict__ == other.__dict__ | |||
|
|
|||
| def __str__(self) -> str: | |||
| return self.shortName | |||
| return f"{self.shortName}@{self.version}" | |||
jenkinsapi/plugins.py
Outdated
| def update_center_dict(self): | ||
| update_center = "https://updates.jenkins.io/update-center.json" | ||
| jsonp = requests.get(update_center).content.decode("utf-8") | ||
| return json.loads(jsonp_to_json(jsonp)) | ||
| if not hasattr(self, "_update_center_dict"): | ||
| update_center = ( | ||
| "https://updates.jenkins.io/update-center.actual.json" | ||
| ) | ||
| self._update_center_dict = requests.get(update_center).json() | ||
| return self._update_center_dict | ||
|
|
||
| @property | ||
| def update_center_dict_server(self): | ||
| if not hasattr(self, "_update_center_dict_server"): | ||
| self._update_center_dict_server = self.jenkins_obj.requester.get_url( | ||
| self.jenkins_obj.get_update_center_url(2) | ||
| ).json() | ||
| return self._update_center_dict_server |
There was a problem hiding this comment.
I am concerned about this part, and anywhere I added caching. as it may have impact on code that uses Plugins.update_center_dict and expect it to make a call.
There was a problem hiding this comment.
When in doubt, lets add more tests
I am trying hard to steer away from breaking changes
I'll take another look when I have time
Hi,
Please find here some improvements added to this, very good, library.
In order to use it efficiently and correctly, I had to integrate these updates.
I cherry-picked (to give credit to @mrrsm) the commit added in this PR.
I hope this can be merged, so I can switch to the real release, instead of my own repo.