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

Two blank lines after an import should be reduced to one #4489

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

kastkeepitjumpinlikekangaroos

Description

Hey! Trying to contribute for the first time to the project so let me know if I'm missing anything :)

I've implemented changes to address this issue #2020 where there's some inconsistency around how many lines are added after an import. I've added 2 test cases showing that if there is more than 1 blank line between the import and the next line that it is now being reduced to one. One test case was failing because it had in its expected outout 2 lines between the import and following statement, so I think modifying the test case now that it should always be one makes sense.

Checklist - did you ...

  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

src/black/lines.py Outdated Show resolved Hide resolved
@kastkeepitjumpinlikekangaroos
Copy link
Author

I got an email about this job failure but looks like all the checks are passing on the PR https://github.com/psf/black/actions/runs/11377698063/job/31652281800 is this something that needs to be addressed?

@JelleZijlstra
Copy link
Collaborator

Interesting, I suspect some change to how GitHub handles artifacts. Not sure why it doesn't show up here.

@kastkeepitjumpinlikekangaroos
Copy link
Author

kastkeepitjumpinlikekangaroos commented Oct 25, 2024

I got an email about this job failure but looks like all the checks are passing on the PR https://github.com/psf/black/actions/runs/11377698063/job/31652281800 is this something that needs to be addressed?

I got this same error again on the rerun of the workflows https://github.com/psf/black/actions/runs/11483099904/job/31957735676 and seems to be because there's no artifact in this workflow https://github.com/psf/black/actions/runs/11412528080 with the file name .pr-comment.json. If I'm understanding correctly it looks like this is because these tasks which call comment-body (which produces .pr-comment.json https://github.com/psf/black/blob/main/scripts/diff_shades_gha_helper.py#L167) only executes when the matrix mode is preview-changes and the above diff-shades workflow has a matrix mode of analysis

https://github.com/psf/black/blob/main/.github/workflows/diff_shades.yml#L130:

      - name: Generate summary file (PR only)
        if: github.event_name == 'pull_request' && matrix.mode == 'preview-changes'
        run: >
          python helper.py comment-body
          ${{ matrix.baseline-analysis }} ${{ matrix.target-analysis }}
          ${{ matrix.baseline-sha }} ${{ matrix.target-sha }}
          ${{ github.event.pull_request.number }}

      - name: Upload summary file (PR only)
        if: github.event_name == 'pull_request' && matrix.mode == 'preview-changes'
        uses: actions/upload-artifact@v3
        with:
          name: .pr-comment.json
          path: .pr-comment.json

@kastkeepitjumpinlikekangaroos
Copy link
Author

hey @JelleZijlstra ! :) just bumping this PR - do the changes look good or are there other modifications you'd like to see?

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks, this looks reasonable

One nit: could you move the logic down next to

black/src/black/lines.py

Lines 674 to 680 in c98fc0c

if (
self.previous_line.is_import
and not current_line.is_import
and not current_line.is_fmt_pass_converted(first_leaf_matches=is_import)
and depth == self.previous_line.depth
):
return (before or 1), 0
and also add a test case for when imports are within a function?

@kastkeepitjumpinlikekangaroos
Copy link
Author

Thanks, this looks reasonable

One nit: could you move the logic down next to

black/src/black/lines.py

Lines 674 to 680 in c98fc0c

if (
self.previous_line.is_import
and not current_line.is_import
and not current_line.is_fmt_pass_converted(first_leaf_matches=is_import)
and depth == self.previous_line.depth
):
return (before or 1), 0

and also add a test case for when imports are within a function?

@hauntsaninja for sure, let me know if where I moved the logic is what you were thinking!

also moved all the case cases to a single file, added a test case for imports inside a function, and expanded some existing test cases

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.

3 participants