Modify models/dag.py to let it pass pylint check #16584
Conversation
buxizhizhoum
requested review from
ashb,
kaxil,
potiuk,
turbaszek and
XD-DENG
as code owners
|
@ashb Could you have a look on this? |
airflow/models/dag.py
Outdated
| # self.log.debug(e) | ||
| # d['is_picklable'] = False | ||
| # d['stacktrace'] = traceback.format_exc() | ||
| # return d |
There was a problem hiding this comment.
I searched the project, it seems that this method is not used already, Or may be I am wrong?
There was a problem hiding this comment.
You might as well be right (there are a lot of dead code in Airflow). But we can’t just remove it due to backward compatibility. Maybe add a DeprecationWarning (to plan for a removal) and ignore the entire block?
There was a problem hiding this comment.
It is better than what I did, thanks for the advice.
There was a problem hiding this comment.
I have placed a DeprecationWarning to this method, and applied some change to suppress pylint error.
airflow/models/dag.py
Outdated
| @@ -2122,7 +2165,7 @@ class DagTag(Base): | |||
| dag_id = Column(String(ID_LEN), ForeignKey('dag.dag_id'), primary_key=True) | |||
|
|
|||
| def __repr__(self): | |||
| return self.name | |||
| return f"<DagTag: {self.name}>" | |||
There was a problem hiding this comment.
This change doesn’t seem related.
There was a problem hiding this comment.
Yes, I will rollback this.
There was a problem hiding this comment.
This change has been rolled back, the original return self.name triggers an pylint error(repr does not return str (invalid-repr-returned)), and I change it a little to suppress it.
|
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
github-actions
bot
added
the
full tests needed
airflow/models/dag.py
Outdated
| self._full_filepath = value | ||
|
|
||
| @property | ||
| def concurrency(self) -> int: | ||
| """Max active tasks limit of a dag""" |
There was a problem hiding this comment.
I find these type of DAG comments less helpful than no doc at all.
I'd rather we ignored the warning on these ones.
There was a problem hiding this comment.
I have removed less helpful doc strings including this one, thanks for your advice:)
There was a problem hiding this comment.
@ashb Could you help to have a look again?
buxizhizhoum
force-pushed
the
main
branch
3 times, most recently
from
5f196e3
to
b80384d
Compare
|
Some static checks/docs/pylint still fail |
|
|
Still some errors. I HEARTILY recommend installing pre-commits locally and running them for every commit. |
|
(and runing docs build locally is also fast now) |
my bad, I will fix as soon as possible. |
|
Btw. We are getting rid of puking today (see the devlist) so this change is really obsolete. |
|
shall we abandon this in the light of #16682 ? |
sure |
Modified the airflow/models/dag.py model to let it pass static code check