When moving a class out into its own file should the corresponding tests also be moved out of tests/models.py?
I'd say yes, that file already has more then 3200 lines. Kind Regards, Stefan On 12/17/18 8:05 PM, Bas Harenslak wrote: > Andreas good idea, without that the only way to refactor models.py is a big > bang all at once. > > I made a start and renamed models.py to /airflow/models/__init__.py and moved > class Connection to /airflow/models/connection.py (AIRFLOW-3458). PR is here: > https://github.com/apache/incubator-airflow/pull/4335. Still waiting for all > tests to complete. If this works okay, I could continue and split off other > classes. > > Once the last PR of Fokko’s list is merged we should ensure > /airflow/models/__init__.py is empty. > > Also like Tao Feng says, it’s indeed wise to close as many PRs as possible > first. > > On 17 Dec 2018, at 19:59, Tao Feng > <fengta...@gmail.com<mailto:fengta...@gmail.com>> wrote: > > We should merge the existing prs before doing this refactors. Otherwise, > there will be so many rebase issues. > > On Mon, Dec 17, 2018 at 12:37 AM Andreas Költringer < > andreas.koeltrin...@n-fuse.co<mailto:andreas.koeltrin...@n-fuse.co>> wrote: > > Hi, > > a suggestion to make this process easier: > > a folder could be created called `models`. The `models.py` could then > moved > into that folder and renamed to `__init__.py`. That way, all the other > parts > of airflow can be left untouched - it is still possible to run > > `from models import <something>` > > In the next steps, classes can be moved out of the now called > `__init__.py` > into separate files. The 'moved' class then needs to be imported in > `__init__.py` - to not affect the rest of airflow. > > Example: move class `User` to `models/user.py`. In `models/__init__.py` > add > `from .user import User`. > > What do you think? > > > On Thursday, December 6, 2018 1:08:34 PM CET Ash Berlin-Taylor wrote: > Hi Fokko, > > I commented on some of the issues -we could possibly just delete User and > KnownEvent* > My suggestion is to create a new package, called models, which will > contain all the orm classes > And do what with the current airflow.models? > > Do you have an idea of module names to move things to? Are you thinking > we > have airflow.models.connection module containing just a Connection class > for example? > > -ash > > On 6 Dec 2018, at 11:35, Driesprong, Fokko > <fo...@driesprong.frl<mailto:fo...@driesprong.frl>> > wrote: > > Hi All, > > I think it is time to refactor the infamous models.py. This file is far > too > big, and it only keeps growing. My suggestion is to create a new > package, > called models, which will contain all the orm classes (the ones > with __tablename__ in the class). And for example the BaseOperator to > the > operator packages. I've created a lot of tickets to move the classes > one > by > one out of models.py. The reason to do this one by one is to relieve > the > pain of fixing the circular dependencies. > > Refactor: Move DagBag out of models.py > https://issues.apache.org/jira/browse/AIRFLOW-3456 > > Refactor: Move User out of models.py > https://issues.apache.org/jira/browse/AIRFLOW-3457 > > Refactor: Move Connection out of models.py > https://issues.apache.org/jira/browse/AIRFLOW-3458 > > Refactor: Move DagPickle out of models.py > https://issues.apache.org/jira/browse/AIRFLOW-3459 > > Refactor: Move TaskInstance out of models.py > https://issues.apache.org/jira/browse/AIRFLOW-3460 > > Refactor: Move TaskFail out of models.py > https://issues.apache.org/jira/browse/AIRFLOW-3461 > > Refactor: Move TaskReschedule out of models.py > https://issues.apache.org/jira/browse/AIRFLOW-3462 > > Refactor: Move Log out of models.py > https://issues.apache.org/jira/browse/AIRFLOW-3463 > > Refactor: Move SkipMixin out of models.py > https://issues.apache.org/jira/browse/AIRFLOW-3464 > > Refactor: Move BaseOperator out of models.py > https://issues.apache.org/jira/browse/AIRFLOW-3465 > > Refactor: Move DAG out of models.py > https://issues.apache.org/jira/browse/AIRFLOW-3466 > > Refactor: Move Chart out of models.py > https://issues.apache.org/jira/browse/AIRFLOW-3467 > > Refactor: Move KnownEventType out of models.py > https://issues.apache.org/jira/browse/AIRFLOW-3468 > > Refactor: Move KnownEvent out of models.py > https://issues.apache.org/jira/browse/AIRFLOW-3469 > > Refactor: Move Variable out of models.py > https://issues.apache.org/jira/browse/AIRFLOW-3470 > > Refactor: Move XCom out of models.py > https://issues.apache.org/jira/browse/AIRFLOW-3471 > > Refactor: Move DagStat out of models.py > https://issues.apache.org/jira/browse/AIRFLOW-3472 > > Refactor: Move DagRun out of models.py > https://issues.apache.org/jira/browse/AIRFLOW-3473 > > Refactor: Move SlaMiss out of models.py > https://issues.apache.org/jira/browse/AIRFLOW-3474 > > Refactor: Move ImportError out of models.py > https://issues.apache.org/jira/browse/AIRFLOW-3475 > > Refactor: Move KubeResourceVersion out of models.py > https://issues.apache.org/jira/browse/AIRFLOW-3476 > > Refactor: Move KubeWorkerIdentifier out of models.py > https://issues.apache.org/jira/browse/AIRFLOW-3477 > > Some classes are really simple, and would also be a nice opportunity > for > newcomers to start contributing to Airflow :-) > > Cheers, Fokko > > > -- > Andreas Koeltringer > Mail: andreas.koeltrin...@n-fuse.co<mailto:andreas.koeltrin...@n-fuse.co> > > > > > >
