Page MenuHomePhabricator

Reorganizing Lexeme code
Closed, ResolvedPublic

Description

Organizing code in Wikibase Lexeme by type/layer so dependencies become clearer, simple guidelines can be created and architectural boundaries can be worked towards more easily.

Initial idea (incomplete and will inevitably be superseded by better understanding as we go along):

src/
    Domain/ -- "business code" (no code specific to a single Interactor), probably no MW binding here
        Model/  -- DataModel classes, definitely independent from MW
        Persistence/ -- Lookups, repositories, etc (interfaces, decorators and trivial implementations) (not sure about the name. Perhaps one folder for Lookups/ and one for Repositories/)
        Merge/
        Diff/
        .../ -- Other coherent sets of domain services
    Interactors/ -- "application code" (code specific to a single Interactor), probably no MW binding here
        AddLexeme/
        MergeLexemes/ -- code specific to the Interactor
            MergeLexemeInteractor.php
            MergeLexemeRequest.php -- typical structure for new Interactors
            MergeLexemeResponse.php -- typical structure for new Interactors. Can also be a MergeLexemePresenter (interface) in case of fractured response model
            SupportingApplicationCodeService.php -- cannot be used by anything but the Interactor
        .../ -- Other Interactors
    DataAccess/ -- persistence implementations (current Store/ folder, perhaps keep that name)
    MediaWiki/ -- implementations of MW hook mechanisms
        Actions/
        Api/
        Content/
        Specials/
    .../ -- Remaining things as they are now, or perhaps organized further

Some random thoughts related to this structure:

  • The DataModel/ folder is no longer called DataModel. This because I think it is good to move away from plain data structures and include domain logic. For instance a TermList can hold to logic to merge in another TermList, rather than having a TermListMergingService. Some things will still warrant a service, and responsibilities such as persistence are definitely best kept in Repositories and the like.
  • Current Interactors do not have request and response models. Normally it is good to have these. When there are dedicated Interactor folders you have a more natural place to put these.
  • This structure is a step towards being able to strongly separate MW independent code. Ideally the Domain/ and Interactors/ folders do not contain any framework binding.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 10 2018, 11:08 AM
Restricted Application added a project: Wikidata. · View Herald TranscriptOct 10 2018, 11:14 AM
Addshore triaged this task as Medium priority.Oct 11 2018, 8:41 AM
Addshore moved this task from incoming to in progress on the Wikidata board.
This comment was removed by JeroenDeDauw.
JeroenDeDauw added a comment.EditedOct 11 2018, 4:40 PM

Initial moving is done, ending with https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WikibaseLexeme/+/466691

The linked commit is the only one that does something other than directory/NS move. These moves have been done with the move refactoring for single classes or search and replace for directories.

After this I want to look at things in a bit more detail, likely doing some further moving and maybe doing some strategic refactoring to get rid of anomalous binding.

Note that I did not move any of the tests yet. Will do that once things are settled a bit more.

I've started cleaning up the existing interactor to correspond to what an interactor normally is and to respect architectural boundaries. This is mostly done. Chain of commits starting with https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WikibaseLexeme/+/469373

This task hasn't been touched or updated in 7 days.
@JeroenDeDauw what is the status of this?

The initial pass over the structure is done. Having a single mostly-sane UC is almost done. See https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WikibaseLexeme/+/472159 and dependent commits. After that the tests will also need to be reorganized. I do not need this ticket to keep track of that, so it can be closed if you want.

@JeroenDeDauw can this be moved to done now?

Ping. Let's please get this unstuck.

So there is one bit of work that still needs to be done

Note that I did not move any of the tests yet. Will do that once things are settled a bit more.

@JeroenDeDauw are you going to be able to finish this up any time soon?

Addshore claimed this task.Jan 11 2019, 3:32 PM
Restricted Application added a project: User-Addshore. · View Herald TranscriptJan 11 2019, 3:32 PM

Just had a chat with @JeroenDeDauw on IRC and he is taking this one back today :)

JeroenDeDauw added a comment.EditedJan 14 2019, 12:12 PM

I think it is valuable to have the separation between MW and non-MW bound code on a high level, rather than repeating it in src/ and tests/. Might also be good to separate JS and PHP more.

-- Approach 1

WikibaseLexeme/
    extension/
        i18n/
        maintenance/
        resources/
        includes/ -- Contents of current src/MediaWiki/
            Actions/
            Api/
            ...
        tests/
            browser/
            jasmine/
            qunit/
            phpunit/
            selenium/
        WikibaseLexeme.php
        ...
    context/ -- Fully MW independent
        src/ -- Same as current src/ but without src/MediaWiki/
            Example/
                SomeClass.php
        tests/
            unit/
                Example/
                    SomeClassTest.php
            integration/
            fixtures/
-- Approach 2: also seperating js and php in extension/

WikibaseLexeme/
    extension/
        i18n/
        php/
            maintenance/
            resources/
            includes/ -- Contents of current src/MediaWiki/
                Actions/
                Api/
                ...
            tests/
                unit/
                integration/
                system/
                fixtures/
        js/
            src/ -- Contents of current resources/
            tests/ -- Contents of current tests/qunit, perhaps also tests/jasmine?
        WikibaseLexeme.php
        ...
    context/ -- Same as approach 1

I'm not up to speed with the state of the JS. If there is a lot of non-MW specific stuff in there, then it could make sense to have a JS thing on the top level next to extension/ and context/.

context/ might also not be a super name. Can't think of anything concise that would be more clear and accurate though.

Feedback please!

I honestly don't know which one of these will end up being the best in the long run.
As the JS code is not actually being split up, then the JS tests probably should just remain in a JS directory for now.

alaa_wmde moved this task from Unsorted to Separate concerns on the Technical-Debt board.
alaa_wmde added a subscriber: alaa_wmde.

Moving out of campsite iteration for now as it is not being worked on for a while now. Also has been added to Technical-Debt board under "Separate Concerns" (maybe there's a better column for it there)

Let's estimate and prioritize this in story time, and take it to campsite to finish it.

The only requirement left before this can be resolved as done is:

  • Restructure /test folder according to the new source structure under /src
alaa_wmde moved this task from In Progress to Ready to estimate on the Wikidata-Campsite board.
alaa_wmde closed this task as Resolved.Jul 2 2019, 12:41 PM
alaa_wmde claimed this task.

Resolved as /test structure might need to change anyway once mediawiki unit/integration refactoring work is done T87781: Split mediawiki tests into unit and integration tests