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

Shorten max line length to 90 chars #43445

Closed
wants to merge 1 commit into from

Conversation

dstandish
Copy link
Contributor

@dstandish dstandish commented Oct 28, 2024

Shorten max line length to 90 chars for better readability.

I know this one is a long shot, but I actually think it's better readability at 90. The main difference here is that ruff will be more likely to wrap

Example OLD:

@dags_router.get("/{dag_id}/details", responses=create_openapi_http_exception_doc([400, 401, 403, 404, 422]))
async def get_dag_details(
    dag_id: str, session: Annotated[Session, Depends(get_session)], request: Request
) -> DAGDetailsResponse:

Example NEW:

@dags_router.get(
    "/{dag_id}/details",
    responses=create_openapi_http_exception_doc([400, 401, 403, 404, 422]),
)
async def get_dag_details(
    dag_id: str, session: Annotated[Session, Depends(get_session)], request: Request
) -> DAGDetailsResponse:

I think it becomes even better when we specify the args here:

@dags_router.get(
    path="/{dag_id}/details",
    responses=create_openapi_http_exception_doc([400, 401, 403, 404, 422]),
)
async def get_dag_details(
    dag_id: str, session: Annotated[Session, Depends(get_session)], request: Request
) -> DAGDetailsResponse:

@boring-cyborg boring-cyborg bot added the area:API Airflow's REST/HTTP API label Oct 28, 2024
@dstandish dstandish added legacy ui Whether legacy UI change should be allowed in PR legacy api Whether legacy API changes should be allowed in PR labels Oct 28, 2024
@kaxil
Copy link
Member

kaxil commented Oct 28, 2024

Nooo, please don't do this --- not right now 😆

@potiuk
Copy link
Member

potiuk commented Oct 28, 2024

Nooo, please don't do this --- not right now

Nooo, please don't do this --- not right now 😆

😱

BTW. Very interesting - looks like we have source code line number expected in stack-trace in one of the tests:

FAILED tests/models/test_dagbag.py::TestDagBag::test_dabgag_captured_warnings - AssertionError: assert equals failed
  '/opt/airflow/tests/dags/test_d  '/opt/airflow/tests/dags/test_d 
  ag_warnings.py:55: DeprecationW  ag_warnings.py:47: DeprecationW 
  arning: Deprecated Parameter'    arning: Deprecated Parameter'
====== 1 failed, 1934 passed, 913 skipped, 1 warning in 421.03s (0:07:01) ======

@dstandish
Copy link
Contributor Author

Nooo, please don't do this --- not right now 😆

so you are saying.... do it after AIP-72????

@dstandish
Copy link
Contributor Author

Ok closing for now
image

@dstandish dstandish closed this Oct 28, 2024
@mobuchowski
Copy link
Contributor

Please, not something less than 100...

@potiuk
Copy link
Member

potiuk commented Oct 29, 2024

99?

@jedcunningham jedcunningham deleted the shorten-max-line-length branch October 30, 2024 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:API Airflow's REST/HTTP API legacy api Whether legacy API changes should be allowed in PR legacy ui Whether legacy UI change should be allowed in PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants