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

Simplify code with ruff --fix #583

Closed
wants to merge 1 commit into from
Closed

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Feb 28, 2023

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Why not add to pre-commit so it's fixed automatically from now on ?

.github/workflows/test.yml Show resolved Hide resolved
@RonnyPfannschmidt
Copy link
Member

Adding none return annotations to untyped code seems like noise

@cclauss
Copy link
Contributor Author

cclauss commented Feb 28, 2023

I find return None to be a helpful reminder of the possibilities and it does not really slow me down when reading code.
Explicit is better than implicit.

I can revert if that is the consensus.

Or perhaps at line 34, we should:

if not cov_datafile:
    return None

and then dedent the next ~25 lines of code.

@RonnyPfannschmidt
Copy link
Member

The return that was added is great

The type annotations for only the return type none are noise, I'd strongly prefer to have those be added as part of full annotation at a later point

@@ -31,36 +31,34 @@ def init():
cov_branch = True if os.environ.get('COV_CORE_BRANCH') == 'enabled' else None
cov_context = os.environ.get('COV_CORE_CONTEXT')

if cov_datafile:
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this change. I think it makes this block of hairy code worse:

  • harder to git blame
  • conditions are harder to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cclauss cclauss closed this Feb 28, 2023
@cclauss cclauss deleted the ruff-fixes branch February 28, 2023 12:18
@cclauss cclauss restored the ruff-fixes branch February 28, 2023 12:35
@cclauss cclauss deleted the ruff-fixes branch February 28, 2023 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants