You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "xuganyu96 (via GitHub)" <gi...@apache.org> on 2023/05/09 04:21:21 UTC
[GitHub] [airflow] xuganyu96 opened a new pull request, #31141: Parameterize whether input is required for AirflowDateTimePickerWidget
xuganyu96 opened a new pull request, #31141:
URL: https://github.com/apache/airflow/pull/31141
<!--
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/
-->
I'd like to use Airflow's own datetime picker widget, especially when writing web view plugins with FAB. With the existing implementation, the input is always required, but sometimes I have use cases where a datetime value is optional. This pull request made a simple change that parameterize whether the input tag will have the `required` attribute when instantiating an instance of this widget.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.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 [newsfragments](https://github.com/apache/airflow/tree/main/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
[GitHub] [airflow] xuganyu96 commented on a diff in pull request #31141: Parameterize whether input is required for AirflowDateTimePickerWidget
Posted by "xuganyu96 (via GitHub)" <gi...@apache.org>.
xuganyu96 commented on code in PR #31141:
URL: https://github.com/apache/airflow/pull/31141#discussion_r1188889388
##########
airflow/www/widgets.py:
##########
@@ -40,6 +40,9 @@ class AirflowDateTimePickerWidget:
"</div>"
)
+ def __init__(self, input_required: bool = True):
Review Comment:
Thank you for the feedback.
> there can be multiple ways you can enforce an input field to be required
Could you please clarify what other ways you are referring to? The only other place I can think of is with `wtforms` where the forms are validated, but I think these are separate layers of validation that should remain independent.
```python
class PluginWebviewForm(FlaskForm):
required_at_frontend = DateTimeWithTimezoneField(
"Datetime required at frontend",
widget=AirflowDateTimePickerWidget(),
)
required_at_backend = DateTimeWithTimezoneField(
"Datetime required at form validation",
widget=AirflowDateTimePickerWidget(),
validators=[InputRequired()],
)
optional_datetime = DateTimeWithTimezoneField(
"Datetime optional",
widget=AirflowDateTimePickerWidget(input_required=False),
validators=[Optional()],
)
```
What prompted me to make this change is the third field `optional_datetime` in the form where I want to collect an optional datetime.
I did a quick search and found that `AirflowDateTimePicker` originally did not contain the "required" attribute at the input tag. Issue #15976 and pull request #18602 changed the widget so that the `required` attribute is always present, but I think that's too inflexible, and my change aims to make it more flexible.
> if we can see in a single PR the change and its usage
Is this comment sufficient, or should I produce documentation, 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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31141: Parameterize whether input is required for AirflowDateTimePickerWidget
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31141:
URL: https://github.com/apache/airflow/pull/31141#discussion_r1188232782
##########
airflow/www/widgets.py:
##########
@@ -40,6 +40,9 @@ class AirflowDateTimePickerWidget:
"</div>"
)
+ def __init__(self, input_required: bool = True):
Review Comment:
@xuganyu96 Thanks for the contribution! :)
I like the idea of making datetime picker optional, but I foresee an issue, I have not gone through the code, but there can be multiple ways you can enforce an input field to be required, adding the `required` attribute is one of them. So I think adding just this code won't guarantee the behavior for all occurrences. It would be much nicer if we can see in a single PR the change and its usage.
--
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
[GitHub] [airflow] xuganyu96 commented on a diff in pull request #31141: Parameterize whether input is required for AirflowDateTimePickerWidget
Posted by "xuganyu96 (via GitHub)" <gi...@apache.org>.
xuganyu96 commented on code in PR #31141:
URL: https://github.com/apache/airflow/pull/31141#discussion_r1191467683
##########
airflow/www/widgets.py:
##########
@@ -40,6 +40,9 @@ class AirflowDateTimePickerWidget:
"</div>"
)
+ def __init__(self, input_required: bool = True):
Review Comment:
Thank you for the feedback.
I am not entirely familiar with using JS to control attributes of an HTML tag. My intended use case is "outside" the Airflow codebase, such as with developing plugins. For relatively simple plugins it would be great to use widgets with Flask/Jinja2 and not have to work with JS, which is why I want to parameterize the `required` attribute with Python instead of anywhere else.
--
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
[GitHub] [airflow] xuganyu96 closed pull request #31141: Parameterize whether input is required for AirflowDateTimePickerWidget
Posted by "xuganyu96 (via GitHub)" <gi...@apache.org>.
xuganyu96 closed pull request #31141: Parameterize whether input is required for AirflowDateTimePickerWidget
URL: https://github.com/apache/airflow/pull/31141
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31141: Parameterize whether input is required for AirflowDateTimePickerWidget
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31141:
URL: https://github.com/apache/airflow/pull/31141#discussion_r1190718553
##########
airflow/www/widgets.py:
##########
@@ -40,6 +40,9 @@ class AirflowDateTimePickerWidget:
"</div>"
)
+ def __init__(self, input_required: bool = True):
Review Comment:
You can enforce the required clause by just JS as well but again I'm not entirely sure if there are any scenarios in the airflow codebase where that is the case.
IMO, if you plan to use the optional datetime picker anywhere in the code it would give more context if you make those changes in the same PR.
--
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
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #31141: Parameterize whether input is required for AirflowDateTimePickerWidget
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #31141:
URL: https://github.com/apache/airflow/pull/31141#issuecomment-1539373841
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (ruff, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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
[GitHub] [airflow] pierrejeambrun commented on pull request #31141: Parameterize whether input is required for AirflowDateTimePickerWidget
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #31141:
URL: https://github.com/apache/airflow/pull/31141#issuecomment-1563384147
Can you rebase to fix the static check issue please ?
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31141: Parameterize whether input is required for AirflowDateTimePickerWidget
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31141:
URL: https://github.com/apache/airflow/pull/31141#discussion_r1188232782
##########
airflow/www/widgets.py:
##########
@@ -40,6 +40,9 @@ class AirflowDateTimePickerWidget:
"</div>"
)
+ def __init__(self, input_required: bool = True):
Review Comment:
@xuganyu96 Thanks for the contribution! :)
I like the idea of making datetime picker optional, but I foresee an issue, I have not gone through the code(@bbovenzi would be able to confirm :) ), but there can be multiple ways you can enforce an input field to be required, adding the `required` attribute is one of them. So I think adding just this code won't guarantee the behavior for all occurrences. It would be much nicer if we can see in a single PR the change and its usage.
--
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