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

Make static checks generated file more stable across the board #29080

Merged
merged 1 commit into from
Jan 23, 2023

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Jan 21, 2023

There were couple of problems with static checks generating source
files including generated stubs in the common.sql package:

  • black formatting was implemented in multiple separate scripts
    making it harded to fix problems in all of them
  • generated stub files were not formatted with is_pyi=True and
    black had no way to figure it out because it was working on strings
  • black formatting was not consistently applied in all places
  • EOL at the end of generated stub file was missing, leading to EOL
    fixer adding them after generation leading to multiple pre-commit
    passes needed
  • there was (already unused) deprecated dev dict generator that used
    its own black formatting.

There were also couple of problems with the files generated by
stubgen itself:

This PR fixes all the problems:

  • black formatting is now consistenly extracted and applied everywhere
  • when needed, is_pyi flag is passed to black so that it knows
    that .pyi file is being fomratted
  • EOL is added at the end of file when the file is generated
  • Union is added to the generated stub
  • noqa is added to _typeshed import
  • the dict generator is removed

As the end result, generated stub files are fully importable
(no errors reported by IntelliJ IDE) and consistently formatted
every time.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

elif stripped_line.startswith("from _typeshed import Incomplete"):
new_lines.append(line + " # noqa: F401")
elif stripped_line.startswith("from typing import") and "Union" not in line:
new_lines.append(line + ", Union")
Copy link
Member Author

@potiuk potiuk Jan 21, 2023

Choose a reason for hiding this comment

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

Hack. But works (until we have single-line import here of course)

@potiuk
Copy link
Member Author

potiuk commented Jan 21, 2023

WAT ?

@potiuk
Copy link
Member Author

potiuk commented Jan 21, 2023

Strange we have DIFFERENT version here..... aaaaaah I know.

@potiuk potiuk force-pushed the improve-common-sql-stub-generation branch from d8d2400 to 7c33b25 Compare January 21, 2023 12:00
@potiuk potiuk changed the title Improve generation of common.sql stubs Make static checks generated file more stable accross the board Jan 21, 2023
@potiuk
Copy link
Member Author

potiuk commented Jan 21, 2023

OK. I think I solved ALL the problems!

There were couple of problems with static checks generating source
files including generated stubs in the common.sql package:

* black formatting was implemented in multiple separate scripts
  making it harded to fix problems in all of them
* generated stub files were not formatted with is_pyi=True and
  black had no way to figure it out because it was working on strings
* black formatting was not consistently applied in all places
* EOL at the end of generated stub file was missing, leading to EOL
  fixer adding them after generation leading to multiple pre-commit
  passes needed
* there was (already unused) deprecated dev dict generator that used
  its own black formatting.

There were also couple of problems with the files generated by
stubgen itself:

* Union was missing in the generated stubs (this is a known issue
  with stubgen: python/mypy#12929
* Intellij complained on Incomplete import from _typeshed missing

This PR fixes all the problems:

* black formatting is now consistenly extracted and applied everywhere
* when needed, is_pyi flag is passed to black so that it knows
  that .pyi file is being fomratted
* EOL is added at the end of file when the file is generated
* Union is added to the generated stub
* noqa is added to _typeshed import
* the dict generator is removed

As the end result, generated stub files are fully importable
(no errors reported by IntelliJ IDE) and consistently formatted
every time.
@potiuk potiuk force-pushed the improve-common-sql-stub-generation branch from 7c33b25 to 4451c23 Compare January 21, 2023 12:42
@potiuk potiuk changed the title Make static checks generated file more stable accross the board Make static checks generated file more stable across the board Jan 22, 2023
.pre-commit-config.yaml Show resolved Hide resolved
@potiuk potiuk merged commit 129f082 into apache:main Jan 23, 2023
@potiuk potiuk deleted the improve-common-sql-stub-generation branch January 23, 2023 11:48
potiuk added a commit to potiuk/airflow that referenced this pull request Jan 26, 2023
The recent change apache#29080 introduced missing black import in the
pre-commit that run the compilation. The compilation happened in
the background thread and it's ouptut was only visible in the
asset_compilation output file

This PR fixes the black import problem by removing unnecessary import,
but it will also stop start-airflow and exit with error as well as
surface the output of asset compilation to the console.
potiuk added a commit that referenced this pull request Jan 26, 2023
The recent change #29080 introduced missing black import in the
pre-commit that run the compilation. The compilation happened in
the background thread and it's ouptut was only visible in the
asset_compilation output file

This PR fixes the black import problem by removing unnecessary import,
but it will also stop start-airflow and exit with error as well as
surface the output of asset compilation to the console.
ephraimbuddy pushed a commit that referenced this pull request Mar 8, 2023
There were couple of problems with static checks generating source
files including generated stubs in the common.sql package:

* black formatting was implemented in multiple separate scripts
  making it harded to fix problems in all of them
* generated stub files were not formatted with is_pyi=True and
  black had no way to figure it out because it was working on strings
* black formatting was not consistently applied in all places
* EOL at the end of generated stub file was missing, leading to EOL
  fixer adding them after generation leading to multiple pre-commit
  passes needed
* there was (already unused) deprecated dev dict generator that used
  its own black formatting.

There were also couple of problems with the files generated by
stubgen itself:

* Union was missing in the generated stubs (this is a known issue
  with stubgen: python/mypy#12929
* Intellij complained on Incomplete import from _typeshed missing

This PR fixes all the problems:

* black formatting is now consistenly extracted and applied everywhere
* when needed, is_pyi flag is passed to black so that it knows
  that .pyi file is being fomratted
* EOL is added at the end of file when the file is generated
* Union is added to the generated stub
* noqa is added to _typeshed import
* the dict generator is removed

As the end result, generated stub files are fully importable
(no errors reported by IntelliJ IDE) and consistently formatted
every time.

(cherry picked from commit 129f082)
ephraimbuddy pushed a commit that referenced this pull request Mar 8, 2023
The recent change #29080 introduced missing black import in the
pre-commit that run the compilation. The compilation happened in
the background thread and it's ouptut was only visible in the
asset_compilation output file

This PR fixes the black import problem by removing unnecessary import,
but it will also stop start-airflow and exit with error as well as
surface the output of asset compilation to the console.

(cherry picked from commit c231f11)
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.

2 participants