You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "eumiro (via GitHub)" <gi...@apache.org> on 2023/09/05 17:22:51 UTC
[GitHub] [airflow] eumiro opened a new pull request, #34108: Refactor: Consolidate import and usage of random
eumiro opened a new pull request, #34108:
URL: https://github.com/apache/airflow/pull/34108
(no comment)
--
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] potiuk commented on pull request #34108: Refactor: Consolidate import and usage of random
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #34108:
URL: https://github.com/apache/airflow/pull/34108#issuecomment-1709806138
I think those kind of rules are rarely read and we have two approaches there that 'work'
a) hope that people will follow what is in the code (not perfect but mostly works) - this is a stage we are currently in
b) better - add pre-commit to do ast parsing and add rules of things we want do not want to have. I'd say it would be great if thos unification that can be done (imports are generally easy) are added as a rule to pre-commits
Maybe you can work with @eumiro together to determine the list of rules that can be automatically verified and and write a pre&commit 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #34108: Refactor: Consolidate import and usage of random
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #34108:
URL: https://github.com/apache/airflow/pull/34108#discussion_r1316796894
##########
tests/models/test_cleartasks.py:
##########
@@ -580,25 +581,23 @@ def test_dags_clear(self):
assert tis[i].max_tries == 1
# test only_failed
- from random import randint
-
- failed_dag_idx = randint(0, len(tis) - 1)
- tis[failed_dag_idx].state = State.FAILED
- session.merge(tis[failed_dag_idx])
+ failed_dag = random.choice(tis)
+ failed_dag.state = State.FAILED
+ session.merge(failed_dag)
session.commit()
DAG.clear_dags(dags, only_failed=True)
- for i in range(num_of_dags):
- tis[i].refresh_from_db()
- if i != failed_dag_idx:
- assert tis[i].state == State.SUCCESS
- assert tis[i].try_number == 3
- assert tis[i].max_tries == 1
+ for ti in tis:
Review Comment:
Wow this entire function can use a rewrite. (Not saying you should do it in this 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] eumiro commented on pull request #34108: Refactor: Consolidate import and usage of random
Posted by "eumiro (via GitHub)" <gi...@apache.org>.
eumiro commented on PR #34108:
URL: https://github.com/apache/airflow/pull/34108#issuecomment-1708734684
> Same as #34113. I'm wondering why we want to consolidate imports this way. Does that mean we should avoid `from ... import ...` pattern.
I'm suggesting having all imported modules/classes the same way, so in this case there's always `import random` and not `from random import random` which could confuse devs. `random` is a case where direct `import random` is better, since solitary `random()` or `randrange()` may be obvious, but `sample()` or `choice()` not.
There are cases where I do `from … import …`, e.g. `collections` or `pathlib`. The most important rule is to be repo-wide-consistent. I also usually do `import itertools as it`, but the maintainers suggested to keep it as `itertools`, which I do not object.
--
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] potiuk merged pull request #34108: Refactor: Consolidate import and usage of random
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #34108:
URL: https://github.com/apache/airflow/pull/34108
--
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] Lee-W commented on pull request #34108: Refactor: Consolidate import and usage of random
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #34108:
URL: https://github.com/apache/airflow/pull/34108#issuecomment-1709599113
> > Same as #34113. I'm wondering why we want to consolidate imports this way. Does that mean we should avoid `from ... import ...` pattern.
>
> I'm suggesting having all imported modules/classes the same way, so in this case there's always `import random` and not `from random import random` which could confuse devs. `random` is a case where direct `import random` is better, since solitary `random()` or `randrange()` may be obvious, but `sample()` or `choice()` not.
>
> There are cases where I do `from … import …`, e.g. `collections` or `pathlib`. The most important rule is to be repo-wide-consistent. I also usually do `import itertools as it`, but the maintainers suggested to keep it as `itertools`, which I do not object.
Yep, agree unifying them would be great 👍 Do we want to document it somewhere? (e.g., use `import random` instead of `from random import random`)
--
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