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

style: Perform full linting and fixes to the whole codebase #124

Merged
merged 10 commits into from
Dec 12, 2023

Conversation

lsetiawan
Copy link
Collaborator

Lint and fix the entire codebase to follow the PEP8 Guidelines. Currently the only one check that is turned off is the mypy, which fixes will happen in a separate PR. Pre-commit CI check should now pass.

Remove uneeded shell check precommit. Update codespell settings
to use a whitelist of words. Perform spell checking and fixes.
* style: Update line length config and fix E501 errors

Line length configuration for linting is increased
to 100 in 'pyproject.toml' with 'ruff'.
Performed E501 fixes for anything that is
> 100 line length. This fixes include some
places to have 'noqa: E501' flag.

* style: Fix eof line .codespell-whitelist
* style: Remove * imports, fixes F403 PEP violation

Update all * imports to be more explicit of the
classes/functions being imported, which fixes
PEP8 F403 violation.

* style: Expand imports for data module
Fixes all undefined variables violation of PEP8
F821. Also modified to fix other linting issues.
Copy link

codecov bot commented Dec 11, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (ffb5734) 88.70% compared to head (cfe7c5f) 88.74%.

Files Patch % Lines
src/caustics/data/__init__.py 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #124      +/-   ##
==========================================
+ Coverage   88.70%   88.74%   +0.04%     
==========================================
  Files          36       36              
  Lines        1885     1901      +16     
==========================================
+ Hits         1672     1687      +15     
- Misses        213      214       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@ConnorStoneAstro ConnorStoneAstro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a lot of from x import * are removed, which is fine in general. However it means that maintenance is more work since any new function must be traced through the import hierarchy and added to all the imports and __all__'s. What is the benefit of doing things this way?

src/caustics/__init__.py Show resolved Hide resolved
@lsetiawan
Copy link
Collaborator Author

Great question @ConnorStoneAstro!

PEP8 Standard states the following:

Wildcard imports (from import *) should be avoided, as they make it unclear which names are present in the namespace, confusing both readers and many automated tools. There is one defensible use case for a wildcard import, which is to republish an internal interface as part of a public API (for example, overwriting a pure Python implementation of an interface with the definitions from an optional accelerator module and exactly which definitions will be overwritten isn’t known in advance).

This is typically not a recommended way to import functions and classes as it is very implicit and hard to understand what is actually being imported. One great article that really hammer on this ambiguity can be found at: https://internetworking.dev/why-it-is-bad-idea-to-use-wildcard-based-imports/

However it means that maintenance is more work since any new function must be traced through the import hierarchy and added to all the imports and all's. What is the benefit of doing things this way?

To this point, really what you need is to import any new public functions in a module to the module's __init__.py. I notice that per class for example: src/caustics/light/base.py you have an __all__, this is not necessary and better to put it in the module level. Think of this module level __all__ as specifying exactly what functions or classes that you'd like to expose to your users. I think the benefit here is really readability and easier to follow the code and knowing what's getting exposed vs what's hidden. This allows both users and outside developers to easily understand the important parts of your code.

Normally when programming Python it is always better to be explicit rather than implicit to avoid any confusions, hence their PEP8 above.

Copy link
Member

@ConnorStoneAstro ConnorStoneAstro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great!

@ConnorStoneAstro ConnorStoneAstro merged commit 40fee3e into Ciela-Institute:dev Dec 12, 2023
13 checks passed
@lsetiawan lsetiawan deleted the update_pep8 branch December 12, 2023 17:55
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.

2 participants