Skip to content

Conversation

@bhosale2
Copy link
Collaborator

Fixes #165.

@bhosale2 bhosale2 added enhancement New feature or request prio:low Priority level: low labels Aug 16, 2022
@bhosale2 bhosale2 added this to the Version 0.3.1 milestone Aug 16, 2022
@bhosale2 bhosale2 self-assigned this Aug 16, 2022
@skim0119
Copy link
Collaborator

skim0119 commented Aug 16, 2022

I honestly didn't have good past experience with autoflake. Many intermediate code-push are experimental and work-in-progress, maybe a draft or under review/revision. I preferred flake8 and black because they only touch the formatting without altering the contents itself, but autoflake actually changed the contents. It was a bit more than just a formatting for me.

I'm not sure, and maybe they made it better nowadays. @bhosale2 Any thought? Is there a way to run the check only before merging?

@bhosale2
Copy link
Collaborator Author

@skim0119 I totally understand your concern. That's why I haven't run the formatting yet. Also about the check, the command make check-codestyle will actually show you changes that autoflake will do before formatting (like git diff), and then runs a command that fails if some changes were required by autoflake.

Screen Shot 2022-08-16 at 9 45 12 AM

On the other hand, the make autoflake command is the one that will actually modify files in place.

If you want to ensure safety w.r.t to code changing, should we then only have autoflake in format check and not the actual formatter? Then the user needs to take care of unused imports/vars to fix the check.

@skim0119
Copy link
Collaborator

@bhosale2 Hmm.. how about we only include it in CI? Its developer's responsibility to manage their code while they are developing, but anything that goes to shared branch needs to pass formatted. Although, it reminds me when we didn't have pre-commit for black, and had bunch of commit message saying "black formatting" lol

@bhosale2
Copy link
Collaborator Author

bhosale2 commented Aug 16, 2022

Exactly that's what I meant, we only add it in make check-codestyle which is the CI check, and let the user do the fixing in a sane fashion.

Screen Shot 2022-08-16 at 10 08 41 AM

Ok I will remove autoflake from the make formatting command then, and fix the formatting changes myself.

@armantekinalp
Copy link
Contributor

CI idea sounds good to me, as @skim0119 said developer has to format their code.

@skim0119
Copy link
Collaborator

Can we also include # pragma: no autoflake feature? 👍

@bhosale2
Copy link
Collaborator Author

An update here (after discussion with @skim0119), autoflake seems to remove pass statements after docstrings in empty functions, which are usually important to convey intent. There is some recent ongoing work to allow configurability here, so we will keep this as WIP and fix it in the future when that feature comes in.

@skim0119
Copy link
Collaborator

skim0119 commented Aug 17, 2022

↳ PRs to look for

@bhosale2 bhosale2 requested a review from skim0119 as a code owner August 25, 2022 07:22
@codecov-commenter
Copy link

Codecov Report

Base: 87.53% // Head: 87.53% // No change to project coverage 👍

Coverage data is based on head (f6e54e7) compared to base (435ae35).
Patch has no changes to coverable lines.

Additional details and impacted files
@@              Coverage Diff              @@
##           update-0.3.1     #175   +/-   ##
=============================================
  Coverage         87.53%   87.53%           
=============================================
  Files                44       44           
  Lines              2817     2817           
  Branches            367      367           
=============================================
  Hits               2466     2466           
  Misses              330      330           
  Partials             21       21           
Impacted Files Coverage Δ
elastica/_calculus.py 96.00% <ø> (ø)
elastica/_linalg.py 96.15% <ø> (ø)
elastica/_rotations.py 100.00% <ø> (ø)
elastica/boundary_conditions.py 95.00% <ø> (ø)
elastica/external_forces.py 96.39% <ø> (ø)
elastica/memory_block/memory_block_rigid_body.py 100.00% <ø> (ø)
elastica/modules/base_system.py 98.41% <ø> (ø)
elastica/modules/constraints.py 100.00% <ø> (ø)
...r_block_structure/_reset_ghost_vector_or_scalar.py 100.00% <ø> (ø)
elastica/rigidbody/data_structures.py 44.82% <ø> (ø)
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bhosale2 bhosale2 changed the title [WIP] feat: autoflake funtionality (#165) feat: autoflake funtionality (#165) Aug 25, 2022
@bhosale2 bhosale2 changed the title feat: autoflake funtionality (#165) (#165) feat: autoflake funtionality Aug 25, 2022
Copy link
Collaborator

@skim0119 skim0119 left a comment

Choose a reason for hiding this comment

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

LGTM

@bhosale2 bhosale2 merged commit a129119 into GazzolaLab:update-0.3.1 Aug 25, 2022
@bhosale2 bhosale2 deleted the 165_autoflake branch August 25, 2022 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request prio:low Priority level: low

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Autoflake as a developer dependency for cleaning up unused imports and variables

4 participants