Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix some code quality issues #7494

Merged
merged 3 commits into from Sep 29, 2020
Merged

Conversation

srijan-deepsource
Copy link
Contributor

@srijan-deepsource srijan-deepsource commented Sep 18, 2020

This pull request fixes some of the quality issues raised by DeepSource on my fork of this repository.

Summary of fixes:

  • Remove redundant None default
  • Use identity test to compare types
  • Pass string format arguments as logging method parameters
  • Refactor the comparison involving not
  • Use literal syntax instead of function calls to create a data structure
  • Refactor if expression
  • Iterate dictionary directly
  • Use with statement to open file
  • Merge consecutive isinstance calls

You can see all the issues raised by DeepSource here.
Some of the issues were automatically fixed by DeepSource, while I fixed others manually.

Using DeepSource to continuously analyze your repository

  1. Merge this PR. I have included a .deepsource.toml in this PR, which you can use to configure your analysis settings.
  2. Install DeepSource on your repository here.
  3. Activate analysis here.

If you do not want to use DeepSource to continuously analyze this repo, I'll remove the .deepsource.toml from this PR and you can merge the rest of the fixes.

Changes:

    - Remove redundant `None` default
    - Use identity test to compare types
    - Pass string format arguments as logging method parameters
    - Refactor the comparison involving `not`
    - Use literal syntax instead of function calls to create data structure
    - Refactor `if` expression
    - Iterate dictionary directly
    - Use `with` statement to open file
    - Merge consecutive isinstance calls
Copy link
Member

@stsewd stsewd left a comment

Thanks, not sure about using deepsource. Can you add that in a different PR?

readthedocs/doc_builder/base.py Show resolved Hide resolved
@srijan-deepsource
Copy link
Contributor Author

srijan-deepsource commented Sep 18, 2020

Thanks, not sure about using deepsource. Can you add that in a different PR?

Done. Opened #7495

stsewd
stsewd approved these changes Sep 18, 2020
Copy link
Member

@stsewd stsewd left a comment

Thanks!

@stsewd
Copy link
Member

stsewd commented Sep 23, 2020

Looks like there are some tests that need to be updated.

diff --git a/readthedocs/rtd_tests/tests/test_build_notifications.py b/readthedocs/rtd_tests/tests/test_build_notificati
ons.py
index f09bdffc9..a92131f48 100644
--- a/readthedocs/rtd_tests/tests/test_build_notifications.py
+++ b/readthedocs/rtd_tests/tests/test_build_notifications.py
@@ -24,7 +24,10 @@ class BuildNotificationsTests(TestCase):
     def test_send_notification_none_if_wrong_version_pk(self, mock_logger):
         self.assertFalse(Version.objects.filter(pk=345343).exists())
         send_notifications(version_pk=345343, build_pk=None)
-        mock_logger.warning.assert_called_with("Version not found for given kwargs. {'pk': 345343}")
+        mock_logger.warning.assert_called_with(
+            'Version not found for given kwargs. %s',
+            {'pk': 345343},
+        )


     def test_send_notification_none(self):
diff --git a/readthedocs/rtd_tests/tests/test_celery.py b/readthedocs/rtd_tests/tests/test_celery.py
index aae93822b..90b28ba7e 100644
--- a/readthedocs/rtd_tests/tests/test_celery.py
+++ b/readthedocs/rtd_tests/tests/test_celery.py
@@ -334,7 +334,10 @@ class TestCeleryBuilding(TestCase):
             search_ranking={},
             search_ignore=[],
         )
-        mock_logger.warning.assert_called_with("Version not found for given kwargs. {'pk': 345343}")
+        mock_logger.warning.assert_called_with(
+            'Version not found for given kwargs. %s',
+            {'pk': 345343},
+        )

@stsewd stsewd merged commit e35988f into readthedocs:master Sep 29, 2020
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants