chore: fix code quality issues #1940
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @withshubh on file. In order for us to review and merge your code, please start the CLA process at https://determined.ai/cla. |
There was a problem hiding this comment.
Thank you for offering these improvements to our project! You will have to sign a cla agreement before the PR will be accepted, which is a process you can start here.
The changes from dictionary comprehension to dict() look good.
Please also rename the PR to be chore: ... rather than perf: ..., as the changes proposed will not be any measurable performance impact within our project. The computationally expensive parts of our project are all deep-learning related and would dwarf any performance impact from an extra dictionary comprehension or len() call.
.deepsource.toml
Outdated
| @@ -0,0 +1,14 @@ | |||
| version = 1 | |||
There was a problem hiding this comment.
We appreciate the config, but we are pretty happy with our current static analysis tooling. Please omit this file from the PR.
| @@ -164,7 +164,7 @@ def _write_store(self) -> None: | |||
| self._create_det_path_if_necessary() | |||
| cache_path = self._get_token_cache_path() | |||
| store = {} | |||
| if self._tokens is not None and len(self._tokens): | |||
There was a problem hiding this comment.
Honestly, I think because we use mypy, using on if len(thing): is superior to depending on the truthiness of if thing: because the truthiness test will sometimes hide bugs from mypy if you e.g. change thing to/from Optional[ThingType]. So I'd prefer that these sorts of changes be omitted.
(also if we were going to depend on the truthiness test, the proposed edit is redundant anyway)
withshubh
changed the title
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @deepsourcebot, @deepsource-autofix[bot], @withshubh on file. In order for us to review and merge your code, please start the CLA process at https://determined.ai/cla. |
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @withshubh on file. In order for us to review and merge your code, please start the CLA process at https://determined.ai/cla. |
1 similar comment
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @withshubh on file. In order for us to review and merge your code, please start the CLA process at https://determined.ai/cla. |
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @withshubh on file. In order for us to review and merge your code, please start the CLA process at https://determined.ai/cla. |
|
I have -
Please review 💖 |
|
@cla-bot[bot] check |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
@withshubh Your cla bot is good, but I think you are failing the linting step. You can test it yourself locally by running |
Remove unnecessary use of comprehension Use literal syntax instead of function calls to create data structure Merge `isinstance` calls
My bad. I pushed it by mistake from my local clone. |
|
Thank you @withshubh, I will approve and merge this change after CI passes. |
|
@rb-determined-ai |
|
@withshubh I think you have done a fine job at dev outreach! Honestly I was impressed you followed up after the initial PR review. But the fact is that we are just satisfied with what we have right now, and that our team is already comfortable with. |
Description
Hi 👋 I ran the DeepSource static analyzer on the forked copy of this repo and found some interesting code quality issues. This PR fixes a few of them.
Summary of changes
Remove length check in favour of truthiness of the objectadded .deepsource.toml fileisinstancecalls