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

Use str.splitlines() to split lines #33592

Merged
merged 1 commit into from
Aug 24, 2023
Merged

Conversation

eumiro
Copy link
Contributor

@eumiro eumiro commented Aug 21, 2023

No description provided.

Copy link
Contributor

@amoghrajesh amoghrajesh left a comment

Choose a reason for hiding this comment

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

Changes look good but the CI is failing with the error:

dev/prepare_release_issue.py:304: error:
"Callable[[str, str, SupportsIndex], str]" has no attribute "splitlines" 
[attr-defined]
                    body = " ".join(pr.body.replace.splitlines())
                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~
Found 1 error in 1 file (checked 117 source files)

Have you installed the pre commit? Makes it easier to track static checks..

@eumiro
Copy link
Contributor Author

eumiro commented Aug 22, 2023

@amoghrajesh thanks, I have it installed, but for some reason disabled yesterday. Now it is fixed.

if line
]
if p is not None
lines = ignore_file_path.read_text().splitlines()
Copy link
Member

Choose a reason for hiding this comment

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

I would keep the with open style and rewrite the reading part to

[re2.sub(r"\s*#.*", "", line) for line in ifile]

This would allow the input to be read line by line instead of creating a temporary string before splitting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about

        ignore_file_path = Path(root) / ignore_file_name
        if ignore_file_path.is_file():
            uniq_patterns = {}
            with ignore_file_path.open() as f:
                for line in f:
                    line = line.strip()
                    if line and not line.startswith("#"):
                        pattern = ignore_rule_type.compile(line, Path(base_dir_path), ignore_file_path)
                        if pattern:
                            uniq_patterns[pattern] = None
            # evaluation order of patterns is important with negation
            # so that later patterns can override earlier patterns
            patterns = list(uniq_patterns)

This probably describes the steps even better, and shows that the ignore_rule_type.compile is called before the patterns are filtered via uniq (dict).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I wouldn’t object.

@Taragolis Taragolis merged commit 0e00564 into apache:main Aug 24, 2023
64 checks passed
@ephraimbuddy ephraimbuddy added this to the Airflow 2.7.2 milestone Oct 3, 2023
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Oct 3, 2023
ephraimbuddy pushed a commit that referenced this pull request Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:CLI area:dev-tools kind:documentation provider:cncf-kubernetes Kubernetes provider related issues type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants