Support parsing base-forms parts of a compound word #2
Conversation
mortterna
force-pushed
the
feature/base-form-parts
branch
from
fd19702
to
ab887db
Compare
| @@ -209,6 +213,14 @@ public void setRequireFollowingVerb(@Nullable String requireFollowingVerb) { | |||
| this.requireFollowingVerb = requireFollowingVerb; | |||
| } | |||
|
|
|||
| public void setBaseFormParts(@NotNull List<String> baseFormParts) { | |||
There was a problem hiding this comment.
Tämä voisi varmaan olla konsistentisti @Nullable vaikkei sitä koskaan null-arvoilla kutsuttaisikaan.
| */ | ||
| default @NotNull List<Analysis> analyze(@NotNull CharSequence word, @NotNull AnalyzerConfiguration configuration) { | ||
| return analyze(word, Integer.MAX_VALUE, configuration); | ||
| } |
There was a problem hiding this comment.
Mä ehkä haluaisin sitä että konfiguraatio annettaisiin parametriksi siinä vaiheessa kun analyzer buildataan. Juuri tällä hetkellä konfiguraatiossa ei toki ole mitään sellaista mikä ei voisi olla jo nyt, mutta jos myöhemmin haluttaisiin laajentaa sitä sisältämään vaikka bufferien kokoja tms. niin ei ole kivaa että jouduttaisiin tekemään erillinen konfiguraatioluokka. Muutenkin tuntuu API-mielemmässä selkeämmältä että ensin luodaan konfiguraatiolla sopivanlainen analysaattori ja sitten käytetään sitä.
|
|
||
| package fi.evident.raudikko; | ||
|
|
||
| public class AnalyzerConfiguration { |
There was a problem hiding this comment.
final tähän. Lisäksi voisi mainita pari sanaa siitä että on undefined mennä muuttamaan konfiguraatiota sen jälkeen kun sen on antanut analysaattorille.
| @@ -165,6 +168,18 @@ else if (!classTagSeen && tag.matches(Tags.lu)) { | |||
| return baseform.toString(); | |||
| } | |||
|
|
|||
| static @NotNull List<String> parseBaseFormParts(@NotNull SymbolBuffer tokenizer) { | |||
| List<String> parts = new ArrayList<>(4); | |||
There was a problem hiding this comment.
Oletuskapasiteetti on 10, joten parhaassa tapauksessa säästetään (10-4)*8=48 byten allokointi tietorakenteesta joka jää hyvin todennäköisesti väliaikaiseksi ja heitetään pian menemään. Huonoimmassa tapauksessa aiheuttaa toisen allokaation.
Jos tässä näkee jonkun initial-sizen, niin olisi kiva nähdä joku benchmark, jonka mukaan sillä on mitään vaikutusta. Mutta tottahan sekin on, että eihän oikeassa tekstissä mitään lentokonesuihkuturbiinimoottoriapumekaanikkoaliupseerioppilaita tule vastaan. Että ihan miten vain. 😄
| this.includeFstOutput = includeFstOutput; | ||
| } | ||
|
|
||
| public static AnalyzerConfiguration all() { |
There was a problem hiding this comment.
I can has @NotNull?
Makustelin tälle myös nimeä full, mutta en ole varma kumpi on parempi.
Sitten mietin että jos nyt on vaihtoehtona staattinen factory metodi tai constructor ja molemmat tekevät saman asian, mutta constructorin toimintaa ei ole dokumentoitu, niin ehkä sen pitäisi olla private?
Sitten pohdin vielä sitäkin että onko all/full nimi mihin halutaan sitoutua tulevaisuudessa. Mitäpä jos joskus tuleekin joku super-raskas uusi analyysi eikä haluta ylläreitä olemassaoleville käyttäjille niin että ne saavat sen automaattisesti mukaan kun päivittävät uuteen versioon.
Eli ehkä sittenkin joku default tms? Mutta olisiko sitten kuitenkin ihan vain se default-constructor? Luokan javadocissa voisi mainita että oletuksena saa sellaiset "järkevät default-asetukset, jotka sisältävät suurimman osan analyyseistä" tms
| @@ -65,6 +66,9 @@ public final class Analysis implements Cloneable { | |||
| private boolean possibleGeographicalName = false; | |||
| private @Nullable String requireFollowingVerb; | |||
|
|
|||
| // Data extended in comparison to original libvoikko implementation | |||
There was a problem hiding this comment.
Ei tätä varmaan tarvitse erikseen mainita.
| @@ -277,6 +289,7 @@ public String toString() { | |||
| ", malagaVapaaJalkiosa=" + malagaVapaaJalkiosa + | |||
| ", possibleGeographicalName=" + possibleGeographicalName + | |||
| ", requireFollowingVerb='" + requireFollowingVerb + '\'' + | |||
| ", baseFormParts='" + (baseFormParts != null ? String.join(", ", baseFormParts) : null) + '\'' + | |||
There was a problem hiding this comment.
Listan toString on itsessään mainio, joten tää voi olla vain:
| ", baseFormParts='" + (baseFormParts != null ? String.join(", ", baseFormParts) : null) + '\'' + | |
| ", baseFormParts='" + baseFormParts + '\'' + |
| configuration.setIncludeBaseForm(true); | ||
| configuration.setIncludeBasicAttributes(false); | ||
| configuration.setIncludeOrganizationNameAnalysis(false); | ||
| configuration.setIncludeFstOutput(false); |
There was a problem hiding this comment.
Nyt kun lisätään uusia analyysejä, pitää muistaa päivittää tätä. Ehkä tässä voisi olla:
AnalyzerConfiguration configuration = AnalyzerConfiguration.none();
configuration.setIncludeBaseForm(true);tai jopa:
AnalyzerConfiguration configuration = AnalyzerConfiguration.onlyBaseForm();Mutta jos nyt lähdetään siitä että konfiguraatio on annettu jo analyzeria luodessa ja se voisi potentiaalisesti sisältää ihan muitakin asetuksia kuin ne mitä nykyään on, niin eihän me voida ylipäätään vain luoda konfiguraatiota tyhjästä, koska se ei välttämättä ole lainkaan sitä käyttäjä haluaa.
Joka tapauksessa tämä metodi on sellainen joka oikeassa implementaatiossa on overridettu tehokkaammalla, joten pidetään tämä ennallaan: kutsutaan vain analyze-metodia normaalisti ja keräillään sieltä baseformit talteen luottaen että käyttäjä on antanut konfiguraation, jossa baseformit halutaan.
mortterna
force-pushed
the
feature/base-form-parts
branch
from
ab887db
to
fb25cc3
Compare
komu
changed the title
No description provided.