You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/08/23 15:46:27 UTC
[GitHub] [airflow] potiuk opened a new pull request #10498: Make www/views.py pylint compatible
potiuk opened a new pull request #10498:
URL: https://github.com/apache/airflow/pull/10498
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10498: Make www/views.py pylint compatible
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10498:
URL: https://github.com/apache/airflow/pull/10498#discussion_r475294374
##########
File path: airflow/www/views.py
##########
@@ -1565,36 +1629,37 @@ def graph(self, session=None):
nodes = []
edges = []
- for task in dag.tasks:
+ for a_task in dag.tasks:
Review comment:
Any proposal ? the original one was overriding task defined above.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10498: Make www/views.py pylint compatible
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10498:
URL: https://github.com/apache/airflow/pull/10498#discussion_r475791804
##########
File path: airflow/www/views.py
##########
@@ -1049,21 +1102,22 @@ def trigger(self, session=None):
dr = DagRun.find(dag_id=dag_id, execution_date=execution_date, run_type=DagRunType.MANUAL)
if dr:
+ # noinspection PyUnresolvedReferences
flash(f"This run_id {dr.run_id} already exists")
return redirect(origin)
run_conf = {}
- conf = request.values.get('conf')
- if conf:
+ the_conf = request.values.get('conf')
Review comment:
Yep.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10498: Make www/views.py pylint compatible
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10498:
URL: https://github.com/apache/airflow/pull/10498#discussion_r475734399
##########
File path: airflow/www/views.py
##########
@@ -151,18 +151,23 @@ def get_date_time_num_runs_dag_runs_form_data(request, session, dag):
# Error handlers
######################################################################################
-def circles(error):
+# noinspection PyUnusedLocal
Review comment:
Is this just for IntelliJ? Pycharm? If yes, can we remove 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10498: Make www/views.py pylint compatible
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10498:
URL: https://github.com/apache/airflow/pull/10498#discussion_r475237654
##########
File path: airflow/www/views.py
##########
@@ -827,23 +873,25 @@ def task(self):
return redirect(url_for('Airflow.index'))
task = copy.copy(dag.get_task(task_id))
task.resolve_template_files()
- ti = TI(task=task, execution_date=dttm)
+ ti = TaskInstance(task=task, execution_date=dttm)
ti.refresh_from_db()
ti_attrs = []
for attr_name in dir(ti):
if not attr_name.startswith('_'):
attr = getattr(ti, attr_name)
- if type(attr) != type(self.task): # noqa
+ if type(attr) != type(self.task): # noqa pylint: disable=unidiomatic-typecheck
Review comment:
```suggestion
if not isinstance(attr, type(self.task)):
```
WDYT?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10498: Make www/views.py pylint compatible
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10498:
URL: https://github.com/apache/airflow/pull/10498#discussion_r475790765
##########
File path: airflow/www/views.py
##########
@@ -94,54 +95,53 @@ def get_safe_url(url):
return url_for('Airflow.index')
-def get_date_time_num_runs_dag_runs_form_data(request, session, dag):
+def get_date_time_num_runs_dag_runs_form_data(the_request, session, dag):
"""Get Execution Data, Base Date & Number of runs from a Request """
- dttm = request.args.get('execution_date')
- if dttm:
- dttm = timezone.parse(dttm)
+ date_time = the_request.args.get('execution_date')
+ if date_time:
+ date_time = timezone.parse(date_time)
else:
- dttm = dag.get_latest_execution_date(session=session) or timezone.utcnow()
+ date_time = dag.get_latest_execution_date(session=session) or timezone.utcnow()
- base_date = request.args.get('base_date')
+ base_date = the_request.args.get('base_date')
if base_date:
base_date = timezone.parse(base_date)
else:
# The DateTimeField widget truncates milliseconds and would loose
# the first dag run. Round to next second.
- base_date = (dttm + timedelta(seconds=1)).replace(microsecond=0)
+ base_date = (date_time + timedelta(seconds=1)).replace(microsecond=0)
default_dag_run = conf.getint('webserver', 'default_dag_run_display_number')
- num_runs = request.args.get('num_runs')
+ num_runs = the_request.args.get('num_runs')
num_runs = int(num_runs) if num_runs else default_dag_run
- DR = models.DagRun
drs = (
- session.query(DR)
+ session.query(DagRun)
Review comment:
We might ad it later indeed :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10498: Make www/views.py pylint compatible
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10498:
URL: https://github.com/apache/airflow/pull/10498#discussion_r475795688
##########
File path: airflow/www/views.py
##########
@@ -2329,28 +2411,29 @@ def process_form(self, form, is_created):
form.extra.data = json.dumps(extra)
def prefill_form(self, form, pk):
+ """Prefill the form."""
try:
- d = json.loads(form.data.get('extra', '{}'))
+ extra_dictionary = json.loads(form.data.get('extra', '{}'))
except JSONDecodeError:
- d = {}
+ extra_dictionary = {}
- if not hasattr(d, 'get'):
- logging.warning('extra field for {} is not iterable'.format(
- form.data.get('conn_id', '<unknown>')))
+ if not hasattr(extra_dictionary, 'get'):
Review comment:
Fixed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10498: Make www/views.py pylint compatible
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10498:
URL: https://github.com/apache/airflow/pull/10498#issuecomment-678792638
Also looks green :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10498: Make www/views.py pylint compatible
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10498:
URL: https://github.com/apache/airflow/pull/10498#discussion_r475237984
##########
File path: airflow/www/views.py
##########
@@ -1565,36 +1629,37 @@ def graph(self, session=None):
nodes = []
edges = []
- for task in dag.tasks:
+ for a_task in dag.tasks:
Review comment:
Hm, this name is a little bit strange to me
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10498: Make www/views.py pylint compatible
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10498:
URL: https://github.com/apache/airflow/pull/10498#discussion_r475238330
##########
File path: airflow/www/views.py
##########
@@ -2329,28 +2411,29 @@ def process_form(self, form, is_created):
form.extra.data = json.dumps(extra)
def prefill_form(self, form, pk):
+ """Prefill the form."""
try:
- d = json.loads(form.data.get('extra', '{}'))
+ extra_dictionary = json.loads(form.data.get('extra', '{}'))
except JSONDecodeError:
- d = {}
+ extra_dictionary = {}
- if not hasattr(d, 'get'):
- logging.warning('extra field for {} is not iterable'.format(
- form.data.get('conn_id', '<unknown>')))
+ if not hasattr(extra_dictionary, 'get'):
Review comment:
Hm, should this be more explicit?
```suggestion
if not isinstance(extra_dictionary, dict):
```
I may be wrong but only `dict` type (from json.loads types) has `get` method?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10498: Make www/views.py pylint compatible
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10498:
URL: https://github.com/apache/airflow/pull/10498#discussion_r475449648
##########
File path: airflow/www/views.py
##########
@@ -94,54 +95,53 @@ def get_safe_url(url):
return url_for('Airflow.index')
-def get_date_time_num_runs_dag_runs_form_data(request, session, dag):
+def get_date_time_num_runs_dag_runs_form_data(the_request, session, dag):
"""Get Execution Data, Base Date & Number of runs from a Request """
- dttm = request.args.get('execution_date')
- if dttm:
- dttm = timezone.parse(dttm)
+ date_time = the_request.args.get('execution_date')
+ if date_time:
+ date_time = timezone.parse(date_time)
else:
- dttm = dag.get_latest_execution_date(session=session) or timezone.utcnow()
+ date_time = dag.get_latest_execution_date(session=session) or timezone.utcnow()
- base_date = request.args.get('base_date')
+ base_date = the_request.args.get('base_date')
if base_date:
base_date = timezone.parse(base_date)
else:
# The DateTimeField widget truncates milliseconds and would loose
# the first dag run. Round to next second.
- base_date = (dttm + timedelta(seconds=1)).replace(microsecond=0)
+ base_date = (date_time + timedelta(seconds=1)).replace(microsecond=0)
default_dag_run = conf.getint('webserver', 'default_dag_run_display_number')
- num_runs = request.args.get('num_runs')
+ num_runs = the_request.args.get('num_runs')
num_runs = int(num_runs) if num_runs else default_dag_run
- DR = models.DagRun
drs = (
- session.query(DR)
+ session.query(DagRun)
Review comment:
Should we add DR, TI and others to unallowed stuff then?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10498: Make www/views.py pylint compatible
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10498:
URL: https://github.com/apache/airflow/pull/10498#discussion_r475236925
##########
File path: airflow/www/views.py
##########
@@ -94,54 +95,53 @@ def get_safe_url(url):
return url_for('Airflow.index')
-def get_date_time_num_runs_dag_runs_form_data(request, session, dag):
+def get_date_time_num_runs_dag_runs_form_data(the_request, session, dag):
"""Get Execution Data, Base Date & Number of runs from a Request """
- dttm = request.args.get('execution_date')
- if dttm:
- dttm = timezone.parse(dttm)
+ date_time = the_request.args.get('execution_date')
+ if date_time:
+ date_time = timezone.parse(date_time)
else:
- dttm = dag.get_latest_execution_date(session=session) or timezone.utcnow()
+ date_time = dag.get_latest_execution_date(session=session) or timezone.utcnow()
- base_date = request.args.get('base_date')
+ base_date = the_request.args.get('base_date')
if base_date:
base_date = timezone.parse(base_date)
else:
# The DateTimeField widget truncates milliseconds and would loose
# the first dag run. Round to next second.
- base_date = (dttm + timedelta(seconds=1)).replace(microsecond=0)
+ base_date = (date_time + timedelta(seconds=1)).replace(microsecond=0)
default_dag_run = conf.getint('webserver', 'default_dag_run_display_number')
- num_runs = request.args.get('num_runs')
+ num_runs = the_request.args.get('num_runs')
num_runs = int(num_runs) if num_runs else default_dag_run
- DR = models.DagRun
drs = (
- session.query(DR)
+ session.query(DagRun)
Review comment:
Was this necessary? Personally I would define `DR` and `TI` on module level to remove a few lines and reduce number of changes at the same time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10498: Make www/views.py pylint compatible
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10498:
URL: https://github.com/apache/airflow/pull/10498#discussion_r475796340
##########
File path: airflow/www/views.py
##########
@@ -151,18 +151,23 @@ def get_date_time_num_runs_dag_runs_form_data(request, session, dag):
# Error handlers
######################################################################################
-def circles(error):
+# noinspection PyUnusedLocal
Review comment:
Indeed. I was too lazy. I replaced all those with 'noqa's and will keep on doing it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10498: Make www/views.py pylint compatible
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10498:
URL: https://github.com/apache/airflow/pull/10498#discussion_r475295182
##########
File path: airflow/www/views.py
##########
@@ -2329,28 +2411,29 @@ def process_form(self, form, is_created):
form.extra.data = json.dumps(extra)
def prefill_form(self, form, pk):
+ """Prefill the form."""
try:
- d = json.loads(form.data.get('extra', '{}'))
+ extra_dictionary = json.loads(form.data.get('extra', '{}'))
except JSONDecodeError:
- d = {}
+ extra_dictionary = {}
- if not hasattr(d, 'get'):
- logging.warning('extra field for {} is not iterable'.format(
- form.data.get('conn_id', '<unknown>')))
+ if not hasattr(extra_dictionary, 'get'):
Review comment:
Could be - but I prefered to have minimal number of actual logic change in this PR and focus on refactoring removing pylint warnings without impacting the logic too much as it would be a bit risky. I'd prefer to leave it as-is. There might be some cases where this object is different than a dict, so i prefer to leave it as is. I think the right time to change it is when we add mypy typing - then we will be more certain of the types of each file, but IMHO that should be a different change. I would love to keep it only focused on naming and other stuff which is pylint-related.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10498: Make www/views.py pylint compatible
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10498:
URL: https://github.com/apache/airflow/pull/10498#discussion_r475293510
##########
File path: airflow/www/views.py
##########
@@ -94,54 +95,53 @@ def get_safe_url(url):
return url_for('Airflow.index')
-def get_date_time_num_runs_dag_runs_form_data(request, session, dag):
+def get_date_time_num_runs_dag_runs_form_data(the_request, session, dag):
"""Get Execution Data, Base Date & Number of runs from a Request """
- dttm = request.args.get('execution_date')
- if dttm:
- dttm = timezone.parse(dttm)
+ date_time = the_request.args.get('execution_date')
+ if date_time:
+ date_time = timezone.parse(date_time)
else:
- dttm = dag.get_latest_execution_date(session=session) or timezone.utcnow()
+ date_time = dag.get_latest_execution_date(session=session) or timezone.utcnow()
- base_date = request.args.get('base_date')
+ base_date = the_request.args.get('base_date')
if base_date:
base_date = timezone.parse(base_date)
else:
# The DateTimeField widget truncates milliseconds and would loose
# the first dag run. Round to next second.
- base_date = (dttm + timedelta(seconds=1)).replace(microsecond=0)
+ base_date = (date_time + timedelta(seconds=1)).replace(microsecond=0)
default_dag_run = conf.getint('webserver', 'default_dag_run_display_number')
- num_runs = request.args.get('num_runs')
+ num_runs = the_request.args.get('num_runs')
num_runs = int(num_runs) if num_runs else default_dag_run
- DR = models.DagRun
drs = (
- session.query(DR)
+ session.query(DagRun)
Review comment:
It was not strictly necessarym but it increases the readability IMHO. It shortens the name but it also increases the mental effort to look up TI and DR elsewhere. I think the code is much more readable when you explicitly spell the class that you use rather than use shorter, two letter names. This is the same reason why pylint disallows short variable names and prefers longer alternatives that spell the actual variable name (df/dtm/fn etc.). That's why I put it as the same change.
In general the code IMHO should be optimised for being more readable explicitely then shorter-writeable.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #10498: Make www/views.py pylint compatible
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #10498:
URL: https://github.com/apache/airflow/pull/10498
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10498: Make www/views.py pylint compatible
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10498:
URL: https://github.com/apache/airflow/pull/10498#discussion_r475293930
##########
File path: airflow/www/views.py
##########
@@ -827,23 +873,25 @@ def task(self):
return redirect(url_for('Airflow.index'))
task = copy.copy(dag.get_task(task_id))
task.resolve_template_files()
- ti = TI(task=task, execution_date=dttm)
+ ti = TaskInstance(task=task, execution_date=dttm)
ti.refresh_from_db()
ti_attrs = []
for attr_name in dir(ti):
if not attr_name.startswith('_'):
attr = getattr(ti, attr_name)
- if type(attr) != type(self.task): # noqa
+ if type(attr) != type(self.task): # noqa pylint: disable=unidiomatic-typecheck
Review comment:
I prefer to keep it as it is. There is a subtle difference between the two statemenets. The original one is true if the two types are the same, the second is true when the attr derives from the "self.task" type (but it can be a subclass). I prefer to stay with the original behaviour even if I have to add the exception.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10498: Make www/views.py pylint compatible
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10498:
URL: https://github.com/apache/airflow/pull/10498#discussion_r475450736
##########
File path: airflow/www/views.py
##########
@@ -1565,36 +1629,37 @@ def graph(self, session=None):
nodes = []
edges = []
- for task in dag.tasks:
+ for a_task in dag.tasks:
Review comment:
How about `dag_task`? In functional languages it common to have `for x in xs`, `for y in ys`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10498: Make www/views.py pylint compatible
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10498:
URL: https://github.com/apache/airflow/pull/10498#discussion_r475237791
##########
File path: airflow/www/views.py
##########
@@ -1049,21 +1102,22 @@ def trigger(self, session=None):
dr = DagRun.find(dag_id=dag_id, execution_date=execution_date, run_type=DagRunType.MANUAL)
if dr:
+ # noinspection PyUnresolvedReferences
flash(f"This run_id {dr.run_id} already exists")
return redirect(origin)
run_conf = {}
- conf = request.values.get('conf')
- if conf:
+ the_conf = request.values.get('conf')
Review comment:
```suggestion
request_conf = request.values.get('conf')
```
Or something more informative also will be good
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10498: Make www/views.py pylint compatible
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10498:
URL: https://github.com/apache/airflow/pull/10498#discussion_r475294290
##########
File path: airflow/www/views.py
##########
@@ -1437,27 +1500,27 @@ def tree(self):
node_count = 0
node_limit = 5000 / max(1, len(dag.leaves))
- def encode_ti(ti: Optional[models.TaskInstance]) -> Optional[List]:
- if not ti:
+ def encode_ti(task_instance: Optional[models.TaskInstance]) -> Optional[List]:
+ if not task_instance:
return None
# NOTE: order of entry is important here because client JS relies on it for
# tree node reconstruction. Remember to change JS code in tree.html
# whenever order is altered.
- data = [
- ti.state,
- ti.try_number,
+ task_instance_data = [
+ task_instance.state,
+ task_instance.try_number,
None, # start_ts
None, # duration
]
- if ti.start_date:
+ if task_instance.start_date:
# round to seconds to reduce payload size
- data[2] = int(ti.start_date.timestamp())
- if ti.duration is not None:
- data[3] = int(ti.duration)
+ task_instance_data[2] = int(task_instance.start_date.timestamp())
+ if task_instance.duration is not None:
+ task_instance_data[3] = int(task_instance.duration)
- return data
+ return task_instance_data
Review comment:
It was overriding ti from module level - defined elsewhere higher in the stack, that's why I renamed it..
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10498: Make www/views.py pylint compatible
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10498:
URL: https://github.com/apache/airflow/pull/10498#discussion_r475451732
##########
File path: airflow/www/views.py
##########
@@ -2329,28 +2411,29 @@ def process_form(self, form, is_created):
form.extra.data = json.dumps(extra)
def prefill_form(self, form, pk):
+ """Prefill the form."""
try:
- d = json.loads(form.data.get('extra', '{}'))
+ extra_dictionary = json.loads(form.data.get('extra', '{}'))
except JSONDecodeError:
- d = {}
+ extra_dictionary = {}
- if not hasattr(d, 'get'):
- logging.warning('extra field for {} is not iterable'.format(
- form.data.get('conn_id', '<unknown>')))
+ if not hasattr(extra_dictionary, 'get'):
Review comment:
> There might be some cases where this object is different than a dict, so i prefer to leave it as is
Any examples? I personally think that we should make pylint changes by fxing code not by adding disables. At least I think that was an assumption made some time ago. I am afraid that if we add disables everywhere then no one ever will fix the code.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10498: Make www/views.py pylint compatible
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10498:
URL: https://github.com/apache/airflow/pull/10498#discussion_r475795588
##########
File path: airflow/www/views.py
##########
@@ -2329,28 +2411,29 @@ def process_form(self, form, is_created):
form.extra.data = json.dumps(extra)
def prefill_form(self, form, pk):
+ """Prefill the form."""
try:
- d = json.loads(form.data.get('extra', '{}'))
+ extra_dictionary = json.loads(form.data.get('extra', '{}'))
except JSONDecodeError:
- d = {}
+ extra_dictionary = {}
- if not hasattr(d, 'get'):
- logging.warning('extra field for {} is not iterable'.format(
- form.data.get('conn_id', '<unknown>')))
+ if not hasattr(extra_dictionary, 'get'):
Review comment:
Actually it was even worse - because the error message was wrong (it was telling about the extra not being iterable.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10498: Make www/views.py pylint compatible
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10498:
URL: https://github.com/apache/airflow/pull/10498#discussion_r475237934
##########
File path: airflow/www/views.py
##########
@@ -1437,27 +1500,27 @@ def tree(self):
node_count = 0
node_limit = 5000 / max(1, len(dag.leaves))
- def encode_ti(ti: Optional[models.TaskInstance]) -> Optional[List]:
- if not ti:
+ def encode_ti(task_instance: Optional[models.TaskInstance]) -> Optional[List]:
+ if not task_instance:
return None
# NOTE: order of entry is important here because client JS relies on it for
# tree node reconstruction. Remember to change JS code in tree.html
# whenever order is altered.
- data = [
- ti.state,
- ti.try_number,
+ task_instance_data = [
+ task_instance.state,
+ task_instance.try_number,
None, # start_ts
None, # duration
]
- if ti.start_date:
+ if task_instance.start_date:
# round to seconds to reduce payload size
- data[2] = int(ti.start_date.timestamp())
- if ti.duration is not None:
- data[3] = int(ti.duration)
+ task_instance_data[2] = int(task_instance.start_date.timestamp())
+ if task_instance.duration is not None:
+ task_instance_data[3] = int(task_instance.duration)
- return data
+ return task_instance_data
Review comment:
Was this necessary? I think `ti` is quite known in Airflow context and we have `ti` as the allowed value in pylinrc.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org