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

Implement no_blank_line_before_class_docstring preview style #9154

Merged
merged 2 commits into from
Dec 19, 2023

Conversation

dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Dec 16, 2023

Summary

This PR implements the no_blank_line_before_class_docstring preview style.

Test Plan

Update existing snapshots.

Formatter ecosystem

main

project similarity index total files changed files
cpython 0.75804 1799 1648
django 0.99984 2772 34
home-assistant 0.99955 10596 213
poetry 0.99905 321 15
transformers 0.99967 2657 324
twine 1.00000 33 0
typeshed 0.99980 3669 18
warehouse 0.99976 654 14
zulip 0.99958 1459 36

dhruv/no-blank-line-docstring

project similarity index total files changed files
cpython 0.75804 1799 1648
django 0.99984 2772 34
home-assistant 0.99955 10596 213
poetry 0.99905 321 15
transformers 0.99967 2657 324
twine 1.00000 33 0
typeshed 0.99980 3669 18
warehouse 0.99976 654 14
zulip 0.99958 1459 36

fixes: #8888

@dhruvmanila dhruvmanila added formatter Related to the formatter preview Related to preview mode features labels Dec 16, 2023
Copy link
Contributor

github-actions bot commented Dec 16, 2023

ruff-ecosystem results

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

ℹ️ ecosystem check encountered format errors. (no format changes; 0 project errors)

demisto/content (+0 -2 lines across 2 files)

ruff format --preview --exclude Packs/ThreatQ/Integrations/ThreatQ/ThreatQ.py

Packs/Automox/Integrations/Automox/Automox.py~L42

 
 
 class Client(BaseClient):
-
     """Client class to interact with the service API
 
     This Client implements API calls, and does not contain any XSOAR logic.

Packs/MS-ISAC/Integrations/MSISAC/MSISAC.py~L13

 
 
 class Client(BaseClient):
-
     """
     This client class for MS-ISAC definies two API endpoints
     Query events in a set amount of days /albert/{days}

freedomofpress/securedrop (+0 -13 lines across 8 files)

ruff format --preview

securedrop/models.py~L336

 
 
 class InvalidUsernameException(Exception):
-
     """Raised when a user logs in with an invalid username"""
 
 

securedrop/models.py~L355

 
 
 class LoginThrottledException(Exception):
-
     """Raised when a user attempts to log in
     too many times in a given time period"""
 
 
 class WrongPasswordException(Exception):
-
     """Raised when a user logs in with an incorrect password"""
 
 
 class PasswordError(Exception):
-
     """Generic error for passwords that are invalid."""
 
 

securedrop/models.py~L387

 
 
 class NonDicewarePassword(PasswordError):
-
     """Raised when attempting to validate a password that is not diceware-like"""
 
 

securedrop/models.py~L840

 
 
 class JournalistLoginAttempt(db.Model):
-
     """This model keeps track of journalist's login attempts so we can
     rate limit them in order to prevent attackers from brute forcing
     passwords or two-factor tokens."""

securedrop/tests/migrations/migration_15ac9509fc68.py~L18

 
 
 class UpgradeTester:
-
     """This migration has no upgrade because there are no tables in the
     database prior to running, so there is no data to load or test.
     """

securedrop/tests/migrations/migration_3d91d6948753.py~L12

 
 
 class UpgradeTester:
-
     """This migration verifies that the UUID column now exists, and that
     the data migration completed successfully.
     """

securedrop/tests/migrations/migration_60f41bb14d98.py~L21

 
 
 class UpgradeTester:
-
     """This migration verifies that the session_nonce column now exists, and
     that the data migration completed successfully.
     """

securedrop/tests/migrations/migration_6db892e17271.py~L77

 
 
 class UpgradeTester:
-
     """This migration verifies that the deleted_by_source column now exists,
     and that the data migration completed successfully.
     """

securedrop/tests/migrations/migration_e0a525cbab83.py~L77

 
 
 class UpgradeTester:
-
     """This migration verifies that the deleted_by_source column now exists,
     and that the data migration completed successfully.
     """

securedrop/tests/migrations/migration_f2833ac34bb6.py~L20

 
 
 class UpgradeTester:
-
     """This migration verifies that the UUID column now exists, and that
     the data migration completed successfully.
     """

securedrop/tests/migrations/migration_fccf57ceef02.py~L32

 
 
 class UpgradeTester:
-
     """This migration verifies that the UUID column now exists, and that
     the data migration completed successfully.
     """

mlflow/mlflow (+0 -2 lines across 2 files)

ruff format --preview

mlflow/langchain/retriever_chain.py~L17

 
 @experimental
 class _RetrieverChain(Chain):
-
     """
     Chain that wraps a retriever for use with MLflow.
 

tests/sagemaker/mock/init.py~L707

 
 
 class TransformJob(TimestampedResource):
-
     """
     Object representing a SageMaker transform job. The SageMakerBackend will create
     and manage transform jobs.

pypa/pip (+0 -2 lines across 2 files)

ruff format --preview

src/pip/_internal/index/collector.py~L395

 
 
 class LinkCollector:
-
     """
     Responsible for collecting Link objects from all configured locations,
     making network requests as needed.

src/pip/_internal/utils/logging.py~L212

 
 
 class ExcludeLoggerFilter(Filter):
-
     """
     A logging Filter that excludes records from a logger (or its children).
     """

pypa/setuptools (+0 -2 lines across 1 file)

ruff format --preview

setuptools/_distutils/version.py~L111

 
 
 class StrictVersion(Version):
-
     """Version numbering for anal retentives and software idealists.
     Implements the standard interface for version number classes as
     described above.  A version number consists of two or three

setuptools/_distutils/version.py~L286

 
 
 class LooseVersion(Version):
-
     """Version numbering for anarchists and software realists.
     Implements the standard interface for version number classes as
     described above.  A version number consists of a series of numbers,

python/mypy (+0 -2 lines across 1 file)

ruff format --preview

mypy/main.py~L342

 
 
 class CapturableArgumentParser(argparse.ArgumentParser):
-
     """Override ArgumentParser methods that use sys.stdout/sys.stderr directly.
 
     This is needed because hijacking sys.std* is not thread-safe,

mypy/main.py~L396

 
 
 class CapturableVersionAction(argparse.Action):
-
     """Supplement CapturableArgumentParser to handle --version.
 
     This is nearly identical to argparse._VersionAction except,

@T-256
Copy link
Contributor

T-256 commented Dec 16, 2023

It seems ecosystem reported numbers are wrong.
Also, links pointed lines aren't exact.

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Could you update your pr summary and include the 'old' compatibility numbers. I don't expect them to change but just to be safe



class Test:
-
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a few tests where we have comments before the docstring (including new or empty lines)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've tested it out locally with different combinations of comments, newline and docstrings. It seems that the Black preview style is a bit inconsistent with the stable style.

Original source code:

class OneBlankLineDocstring:

    """This is a docstring."""


class DocstringWithComment0:
    # This is a comment
    """This is a docstring."""


class DocstringWithComment1:
    # This is a comment

    """This is a docstring."""


class DocstringWithComment2:

    # This is a comment
    """This is a docstring."""


class DocstringWithComment3:

    # This is a comment

    """This is a docstring."""


class DocstringWithComment4:


    # This is a comment


    """This is a docstring."""

Black stable formatting:

class NormalDocstring:

    """This is a docstring."""


class DocstringWithComment0:
    # This is a comment
    """This is a docstring."""


class DocstringWithComment1:
    # This is a comment

    """This is a docstring."""


class DocstringWithComment2:
    # This is a comment
    """This is a docstring."""


class DocstringWithComment3:
    # This is a comment

    """This is a docstring."""


class DocstringWithComment4:
    # This is a comment

    """This is a docstring."""

Black preview formatting:

class OneBlankLineDocstring:
    """This is a docstring."""


class DocstringWithComment0:
    # This is a comment
    """This is a docstring."""


class DocstringWithComment1:
    # This is a comment

    """This is a docstring."""


class DocstringWithComment2:

    # This is a comment
    """This is a docstring."""


class DocstringWithComment3:

    # This is a comment

    """This is a docstring."""


class DocstringWithComment4:

    # This is a comment

    """This is a docstring."""

What I'm noticing is that the rules change if there's a comment in between the class header and docstring in a way that for n1 and n2 number of blank lines before a comment and docstring respectively the following holds true:

  1. If n1 / n2 is 0, keep it 0
  2. If n1 / n2 is 1, keep it 1
  3. If n1 / n2 > 1, then reduce it to 1

It's probably another preview style which is keeping the newline before the comment (allow_empty_first_line_before_block_or_comment).

@charliermarsh
Copy link
Member

The ecosystem line numbers are not guaranteed to be exact (hence the tilde). It’s a diff of two changes against HEAD, so mapping that diff back to a line number in HEAD is not trivial. (Not sure about the “number of lines changed” though.)

@dhruvmanila dhruvmanila force-pushed the dhruv/no-blank-line-docstring branch from 779413d to 2c8c450 Compare December 19, 2023 06:13
@dhruvmanila dhruvmanila merged commit 09296e3 into main Dec 19, 2023
17 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/no-blank-line-docstring branch December 19, 2023 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter preview Related to preview mode features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Formatter: no_blank_line_before_class_docstring preview style
5 participants