zulip / python-zulip-api

Python library for the Zulip API.
https://zulip.com/api/
Apache License 2.0
353 stars 355 forks source link

Fix some code qulity issues #608

Closed pnijhara closed 2 years ago

pnijhara commented 4 years ago

Fixing some of the code quality mentioned in the issue #607.

This patch:

These are some of the antipatterns, bug-risk and performance issues that I have solved in this PR. These issues are solved with the help of DeepSource autofix tool.

As discussed in the issue that many of the issues can be false positives. Those issues can be silenced by ignoring the issues in the issue category in the dashboard.

neiljp commented 4 years ago

From my perspective the primary points worth addressing here would be the empty list default argument case.

I would suggest having each type of change in their own commit, to make review and selection of each easier, though personally I would not consider most of the other changes significant.

pnijhara commented 4 years ago

@neiljp so should I send different PR for different type fo changes and make this one as fixing the default argument case?

andersk commented 4 years ago

Rather than introducing Optional and None where it isn’t needed, I think it’s better to guard against the potential problems with mutable default arguments using read-only mypy types, as I did in zulip/zulip#15349.

-    def create_game_lobby(self, message: Dict[str, Any], users: List[str] = []) -> None:
+    def create_game_lobby(self, message: Dict[str, Any], users: Sequence[str] = []) -> None:

-    def join_game(self, game_id: str, user_email: str, message: Dict[str, Any] = {}) -> None:
+    def join_game(self, game_id: str, user_email: str, message: Mapping[str, Any] = {}) -> None:
pnijhara commented 4 years ago

Ok! So should I revert back these changes from files? @andersk could you please tell me which files do not require this type of change?

pnijhara commented 4 years ago

ping! @andersk Need help!

andersk commented 4 years ago

My comment is about all of the changes of this type, not about particular files.

pnijhara commented 4 years ago

Sure! I will revert this change.

showell commented 3 years ago

@pnijhara Do you still have time to clean up this PR? I think we would need to get the build passing again (I haven't looked to see what's broken) and then do another round of review.

If you don't have time, no worries, we can probably have somebody take this over.

pnijhara commented 3 years ago

@showell I sincerely apologise for the delay. I will revert the suggested change soon.

showell commented 3 years ago

No apology necessary...thanks for the update.

zulipbot commented 3 years ago

Heads up @pnijhara, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

timabbott commented 2 years ago

Closing, since the changes merge-conflict and were generated by a tool, so we'd be better off running the tool again from scratch. Feel free to submit such a PR if you have time again @pnijhara!