alpheios-project / alpheios-core

Alpheios Core Javascript Packages and Libraries
15 stars 2 forks source link

A second round of refactoring changes #539

Closed kirlat closed 3 years ago

kirlat commented 3 years ago

This is an implementation of all the valuable suggestions that I've received during the first PR, and some other improvements as well. It's still not fully production ready and many tests are missing; I'm working on that.

The purpose of this PR is to offer for discussion and finalize a new architecture. Once it's set, I can add reliability improvements whenever necessary to be sure it handles all edge cases well.

The most significant changes are:

Please let me know what do you think. Thanks!

kirlat commented 3 years ago

perhaps we should subdivide them into logical groups? useMorphService, useTreebankData useWordAsLexeme are all only relevant for getLexemes so they could be properties on the getLexemes option treebankProvider, treebankSentenceId, treebankWord ids could be subproperties on useTreebankData lexicons, shortLexicons could be subproperties on getShortDefs

Great suggestion! I hate long lists of parameters. I will update the code accordingly.

kirlat commented 3 years ago

@kirlat , I didn't find any tests for your changes for new workflow for LexicalQuery. And also I could see that some tests are skipped. Do you have an opportunity to create tests for GraphQL workflow? To create some scenarios with different execution paths and see the way it goes through conditions with observable quiery? With different languges?

I'm working on adding more tests right now, I'm planing to merge once the tests will be ready.