You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "aritra24 (via GitHub)" <gi...@apache.org> on 2025/05/03 05:14:56 UTC
[PR] Fix markdown rendering on dag docs [airflow]
aritra24 opened a new pull request, #50142:
URL: https://github.com/apache/airflow/pull/50142
before and after
<img width="958" alt="Screenshot 2025-05-03 at 10 40 33 AM" src="https://github.com/user-attachments/assets/0b7ec02c-a7d9-41c9-817a-0cd40804d95d" />
<img width="958" alt="Screenshot 2025-05-03 at 10 40 24 AM" src="https://github.com/user-attachments/assets/b1be9e84-79aa-4562-ae3e-eb28c3ebf0c0" />
Not sure if there is a case where stripping the whitespace in the front changes the expected experience.
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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 [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix markdown rendering on dag docs [airflow]
Posted by "aritra24 (via GitHub)" <gi...@apache.org>.
aritra24 commented on code in PR #50142:
URL: https://github.com/apache/airflow/pull/50142#discussion_r2077890956
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/dags.py:
##########
@@ -164,6 +165,14 @@ def get_timezone(cls, tz: Timezone | FixedTimezone) -> str | None:
return None
return str(tz)
+ @field_validator("doc_md", mode="before")
+ @classmethod
+ def get_doc_md(cls, doc_md: str | None) -> str | None:
Review Comment:
I'm not opposed to that, it reduces the number of calls to dedent as well. I think the reason behind this was to not effect the serialized db entry.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix markdown rendering on dag docs [airflow]
Posted by "aritra24 (via GitHub)" <gi...@apache.org>.
aritra24 commented on PR #50142:
URL: https://github.com/apache/airflow/pull/50142#issuecomment-2851291627
@pierrejeambrun, moved it to the client side. Let me know what you think now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix markdown rendering on dag docs [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on PR #50142:
URL: https://github.com/apache/airflow/pull/50142#issuecomment-2855130613
Changing the milestone to 3.0.2 -- about to cut 3.0.1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix markdown rendering on dag docs [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #50142:
URL: https://github.com/apache/airflow/pull/50142#issuecomment-2855384062
### Backport successfully created: v3-0-test
<table>
<tr>
<th>Status</th>
<th>Branch</th>
<th>Result</th>
</tr>
<tr>
<td>✅</td>
<td>v3-0-test</td>
<td><a href="https://github.com/apache/airflow/pull/50272"><img src="https://img.shields.io/badge/PR-50272-blue" alt="PR Link"></a></td>
</tr>
</table>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix markdown rendering on dag docs [airflow]
Posted by "aritra24 (via GitHub)" <gi...@apache.org>.
aritra24 commented on code in PR #50142:
URL: https://github.com/apache/airflow/pull/50142#discussion_r2075827152
##########
task-sdk/src/airflow/sdk/definitions/dag.py:
##########
@@ -185,7 +185,18 @@ def _convert_doc_md(doc_md: str | None) -> str | None:
return fh.read()
except FileNotFoundError:
return doc_md
+ return doc_md
+
+def _clean_doc_md(doc_md: str) -> str:
+ # alternative to using math.inf, the linter doesn't like inf being a float
+ strip_count = len(doc_md)
+
+ for line in doc_md.split("\n"):
+ if line != "":
+ strip_count = min(strip_count, len(line) - len(line.lstrip()))
+ if strip_count > 0:
+ doc_md = "\n".join([line[strip_count:] for line in doc_md.split("\n")])
Review Comment:
@pierrejeambrun, moved to api side. let me know if this sounds good to you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix markdown rendering on dag docs [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #50142:
URL: https://github.com/apache/airflow/pull/50142#discussion_r2075484289
##########
task-sdk/src/airflow/sdk/definitions/dag.py:
##########
@@ -185,7 +185,18 @@ def _convert_doc_md(doc_md: str | None) -> str | None:
return fh.read()
except FileNotFoundError:
return doc_md
+ return doc_md
+
+def _clean_doc_md(doc_md: str) -> str:
+ # alternative to using math.inf, the linter doesn't like inf being a float
+ strip_count = len(doc_md)
+
+ for line in doc_md.split("\n"):
+ if line != "":
+ strip_count = min(strip_count, len(line) - len(line.lstrip()))
+ if strip_count > 0:
+ doc_md = "\n".join([line[strip_count:] for line in doc_md.split("\n")])
Review Comment:
Good idea @tirkarthi we can do that on the api server, so we don't modify what's in the db in the first place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix markdown rendering on dag docs [airflow]
Posted by "aritra24 (via GitHub)" <gi...@apache.org>.
aritra24 commented on code in PR #50142:
URL: https://github.com/apache/airflow/pull/50142#discussion_r2075801357
##########
task-sdk/src/airflow/sdk/definitions/dag.py:
##########
@@ -185,7 +185,18 @@ def _convert_doc_md(doc_md: str | None) -> str | None:
return fh.read()
except FileNotFoundError:
return doc_md
+ return doc_md
+
+def _clean_doc_md(doc_md: str) -> str:
+ # alternative to using math.inf, the linter doesn't like inf being a float
+ strip_count = len(doc_md)
+
+ for line in doc_md.split("\n"):
+ if line != "":
+ strip_count = min(strip_count, len(line) - len(line.lstrip()))
+ if strip_count > 0:
+ doc_md = "\n".join([line[strip_count:] for line in doc_md.split("\n")])
Review Comment:
@pierrejeambrun, we wouldn't in that case be able to differentiate between the comment doc vs passing doc directly? Assuming no one uses extra leading spaces in their doc it should be fine...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix markdown rendering on dag docs [airflow]
Posted by "tirkarthi (via GitHub)" <gi...@apache.org>.
tirkarthi commented on code in PR #50142:
URL: https://github.com/apache/airflow/pull/50142#discussion_r2075077969
##########
task-sdk/src/airflow/sdk/definitions/dag.py:
##########
@@ -185,7 +185,18 @@ def _convert_doc_md(doc_md: str | None) -> str | None:
return fh.read()
except FileNotFoundError:
return doc_md
+ return doc_md
+
+def _clean_doc_md(doc_md: str) -> str:
+ # alternative to using math.inf, the linter doesn't like inf being a float
+ strip_count = len(doc_md)
+
+ for line in doc_md.split("\n"):
+ if line != "":
+ strip_count = min(strip_count, len(line) - len(line.lstrip()))
+ if strip_count > 0:
+ doc_md = "\n".join([line[strip_count:] for line in doc_md.split("\n")])
Review Comment:
I just checked in 2.10 and serialized_dag has the spaces preserved in doc_md and on the webserver in Airflow 2 `textwrap.dedent` is used to remove indents. I would prefer using stdlib function than custom implementation.
https://docs.python.org/3/library/inspect.html#inspect.cleandoc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix markdown rendering on dag docs [airflow]
Posted by "aritra24 (via GitHub)" <gi...@apache.org>.
aritra24 commented on code in PR #50142:
URL: https://github.com/apache/airflow/pull/50142#discussion_r2075104420
##########
task-sdk/src/airflow/sdk/definitions/dag.py:
##########
@@ -185,7 +185,18 @@ def _convert_doc_md(doc_md: str | None) -> str | None:
return fh.read()
except FileNotFoundError:
return doc_md
+ return doc_md
+
+def _clean_doc_md(doc_md: str) -> str:
+ # alternative to using math.inf, the linter doesn't like inf being a float
+ strip_count = len(doc_md)
+
+ for line in doc_md.split("\n"):
+ if line != "":
+ strip_count = min(strip_count, len(line) - len(line.lstrip()))
+ if strip_count > 0:
+ doc_md = "\n".join([line[strip_count:] for line in doc_md.split("\n")])
Review Comment:
Oh I wasn't aware of the function, can replace. Do we want to move it away from the serialization location? Is there any merit in having the whitespaces in the db?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix markdown rendering on dag docs [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #50142:
URL: https://github.com/apache/airflow/pull/50142#discussion_r2075484289
##########
task-sdk/src/airflow/sdk/definitions/dag.py:
##########
@@ -185,7 +185,18 @@ def _convert_doc_md(doc_md: str | None) -> str | None:
return fh.read()
except FileNotFoundError:
return doc_md
+ return doc_md
+
+def _clean_doc_md(doc_md: str) -> str:
+ # alternative to using math.inf, the linter doesn't like inf being a float
+ strip_count = len(doc_md)
+
+ for line in doc_md.split("\n"):
+ if line != "":
+ strip_count = min(strip_count, len(line) - len(line.lstrip()))
+ if strip_count > 0:
+ doc_md = "\n".join([line[strip_count:] for line in doc_md.split("\n")])
Review Comment:
Good idea @tirkarthi we can do that on the api server, so we don't modify what's in the db in the first place. (this way the change really has a limited impact)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix markdown rendering on dag docs [airflow]
Posted by "aritra24 (via GitHub)" <gi...@apache.org>.
aritra24 commented on PR #50142:
URL: https://github.com/apache/airflow/pull/50142#issuecomment-2848468867
Yeah, I agree. I wasn't sure what would be the smartest way of trimming. I can move it to the backend and perhaps do a strip min(whitespace at the front of lines)? What do you think @tirkarthi
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix markdown rendering on dag docs [airflow]
Posted by "tirkarthi (via GitHub)" <gi...@apache.org>.
tirkarthi commented on PR #50142:
URL: https://github.com/apache/airflow/pull/50142#issuecomment-2848458986
I would prefer the backend to trim/dedent on docstring during dag parsing than frontend. There could be code blocks with indent in docstring and trimming leading space might cause issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix markdown rendering on dag docs [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #50142:
URL: https://github.com/apache/airflow/pull/50142#discussion_r2074802802
##########
task-sdk/src/airflow/sdk/definitions/dag.py:
##########
@@ -185,7 +185,18 @@ def _convert_doc_md(doc_md: str | None) -> str | None:
return fh.read()
except FileNotFoundError:
return doc_md
+ return doc_md
+
+def _clean_doc_md(doc_md: str) -> str:
+ # alternative to using math.inf, the linter doesn't like inf being a float
+ strip_count = len(doc_md)
+
+ for line in doc_md.split("\n"):
+ if line != "":
+ strip_count = min(strip_count, len(line) - len(line.lstrip()))
+ if strip_count > 0:
+ doc_md = "\n".join([line[strip_count:] for line in doc_md.split("\n")])
Review Comment:
How did it work in 2.10?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix markdown rendering on dag docs [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #50142:
URL: https://github.com/apache/airflow/pull/50142#discussion_r2075484289
##########
task-sdk/src/airflow/sdk/definitions/dag.py:
##########
@@ -185,7 +185,18 @@ def _convert_doc_md(doc_md: str | None) -> str | None:
return fh.read()
except FileNotFoundError:
return doc_md
+ return doc_md
+
+def _clean_doc_md(doc_md: str) -> str:
+ # alternative to using math.inf, the linter doesn't like inf being a float
+ strip_count = len(doc_md)
+
+ for line in doc_md.split("\n"):
+ if line != "":
+ strip_count = min(strip_count, len(line) - len(line.lstrip()))
+ if strip_count > 0:
+ doc_md = "\n".join([line[strip_count:] for line in doc_md.split("\n")])
Review Comment:
Good idea @tirkarthi we can do that on the api server, so we don't modify what's in the db in the first place, the function looks nice (this way the change really has a limited impact)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix markdown rendering on dag docs [airflow]
Posted by "aritra24 (via GitHub)" <gi...@apache.org>.
aritra24 commented on code in PR #50142:
URL: https://github.com/apache/airflow/pull/50142#discussion_r2075801357
##########
task-sdk/src/airflow/sdk/definitions/dag.py:
##########
@@ -185,7 +185,18 @@ def _convert_doc_md(doc_md: str | None) -> str | None:
return fh.read()
except FileNotFoundError:
return doc_md
+ return doc_md
+
+def _clean_doc_md(doc_md: str) -> str:
+ # alternative to using math.inf, the linter doesn't like inf being a float
+ strip_count = len(doc_md)
+
+ for line in doc_md.split("\n"):
+ if line != "":
+ strip_count = min(strip_count, len(line) - len(line.lstrip()))
+ if strip_count > 0:
+ doc_md = "\n".join([line[strip_count:] for line in doc_md.split("\n")])
Review Comment:
@pierrejeambrun, we wouldn't in that case be able to differentiate between the comment doc vs passing doc directly? Assuming no one uses spaces in their doc it should be fine...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix markdown rendering on dag docs [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi merged PR #50142:
URL: https://github.com/apache/airflow/pull/50142
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix markdown rendering on dag docs [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #50142:
URL: https://github.com/apache/airflow/pull/50142#discussion_r2076165093
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/dags.py:
##########
@@ -164,6 +165,14 @@ def get_timezone(cls, tz: Timezone | FixedTimezone) -> str | None:
return None
return str(tz)
+ @field_validator("doc_md", mode="before")
+ @classmethod
+ def get_doc_md(cls, doc_md: str | None) -> str | None:
Review Comment:
Sorry, coming "late" to the party.... and just one tought... would it not be better to "clean" the dedent at time of DAG parsing, not at point of retrieval? That would prevent potential problems of the docs are consumed in other scenarios as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org