osdir.com

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Refactor models.py


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 <fengtao04@xxxxxxxxx<mailto:fengtao04@xxxxxxxxx>> 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.koeltringer@xxxxxxxxx<mailto:andreas.koeltringer@xxxxxxxxx>> 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 <fokko@xxxxxxxxxxxxxx<mailto:fokko@xxxxxxxxxxxxxx>>
> 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.koeltringer@xxxxxxxxx<mailto:andreas.koeltringer@xxxxxxxxx>
> 
> 
> 
> 
> 
>