TIM-JYU / TIM

TIM (The Interactive Material) is an open-source cloud-based platform for creating interactive learning documents.
https://tim.education/view/about/en-US
MIT License
14 stars 4 forks source link

Preamble absolute path - [merged] #3006

Closed dezhidki closed 2 years ago

dezhidki commented 2 years ago

In GitLab by @habca on Jan 21, 2022, 04:38

Merges preamble-absolute-path -> master

Globbaukset ja muut edistyneemmät toiminnot melkein vaativat uuden preambles:-asetuksen.

Testipalvelin: https://timdevs01-6.it.jyu.fi/

dezhidki commented 2 years ago

Voisikohan tuota selkeyttää hieman tekemällä apufunkio, joka jakaa listan kahtia? Mielestäni meillä ei ole sille valmista funktiota, mutta sellaisen saa kasattua itertools-moduulin avulla. Esimerkiksi

TItem = TypeVar("TItem")

def partition(
    pred: Callable[[TItem], bool], iterable: Iterable[TItem]
) -> tuple[Iterable[TItem], Iterable[TItem]]:
    # Create two buffered iterators: values of i1 are cached for i2 to use
    i1, i2 = tee((pred(item), item) for item in iterable)
    return (item for t, item in i1 if t), (item for _, item in i2)

(tuossa TypeVar, Callable ja Iterable on importattuna typing-moduulista; tee on puolestaan itertools-moduulista) Jos sellaisen tekee, se sopisi esimerkiksi timApp/util/utils.py-moduuliin.

Sitten tuon saa vähän selkeämmäksi (ja hieman nopeammaksi, absolute_path kutsutaan vain kerran jokaiselle elementille):

absolute_path_parts, relative_path_parts = parititon(absolute_path, preamble_names)
dezhidki commented 2 years ago

Pythonin set ei säilytä järjestystä, katso esim. tämä SO-vastaus. Meillä on käytössä Python 3.9, eli sen mukainen list(dict.fromkeys(t)) osaa hoitaa duplikaattien poistamisen.

Entä olisiko tässä tarve varmistaa myös, että relative_path_parts:sta tulleet polut eivät toistu list_with_duplicates:ssa olevissa poluissa? Nyt tämä vain poistaa duplikaatit absoluuttisista poluista, muttei duplikaatit, jotka tulevat suhteellisista poluista.

Toisin sanoin voisi kai päästä list_with_duplicates:sta eroon ja lisätä absoluuttiset polut suoraan paths-listaan, sitten lopuksi poistaa kaikki duplikaatit.

dezhidki commented 2 years ago

Appenduksen sijaan voisi ehkä käyttää list_with_duplicates.extend(), niin Pythonin ei tarvitse tehdä jokaisella iteraatiolla aivan uutta listaa.

Lisäksi list(x for x in xs) voi kirjoittaa hieman lyhyemmin muodossa [x for x in xs].

dezhidki commented 2 years ago

Lokaali import voisi siirtää .py-tiedoston alkuun.

dezhidki commented 2 years ago

Tässä sama.

dezhidki commented 2 years ago

Näköjään jos preambles-kansioon tekee preamble-dokumentin, TIM yrittää laittaa preamblen itsensä dokumenttiin, mikä aiheuttaa "The paragraphs in the main document must have distinct ids from the preamble documents." -virheen.

Esimerkki (testuser1): https://timdevs01-6.it.jyu.fi/view/users/test-user-1/test-preambles-self-ref/preambles/preamble

Tällä hetkellä tuotannossa siis preamblet toimivat niin, että templates-kansiossa oleviin dokumentteihin ei laiteta mitään preamblea ja näin vältytään tällaisesta "rekursiivisesta" viittauksesta. Voisi olla periaatteessa niin, että ladattavien preamble-listasta poistetaan aina nykyinen dokumentti itse. Toisaalta voisi tehdä myös niin, että preambles-kansiossa ei näytetä preambleja niin kuin tehdään templates-kansion kanssa nytkin.

Tästä näköjään tulee myös wuffi nyt, kun esikatsellee lohkon preambles/preamble-dokumentissa.

dezhidki commented 2 years ago

In GitLab by @vesal on Jan 21, 2022, 10:43

Miten muuten jos on styles-attribuutti useassa preamblessa, niin summautuvatko ne missä järjestyksessä vai overridaavat toisiaan.

Niistä ehkä pitäisi muodostua yhteinen styles-lista?

dezhidki commented 2 years ago

In GitLab by @vesal on Jan 21, 2022, 11:08

Minusta välttämättä automaattisesti preamblejen ei tarvitse tule muuta kuin siletä templates/preambles-hakemistosta. Sitte on eri juttu jos annetaan ihan absoluuttinen yhtä dokun nimeä vastaava polku

dezhidki commented 2 years ago

In GitLab by @vesal on Jan 21, 2022, 11:19

Onkos tuolla esimerkkejä missä on annettu niitä absoluuttisia preambleja?

Minusta tuolla kaikki:

https://timdevs01-6.it.jyu.fi/view/users/test-user-1

ovat implisiittisiä.

dezhidki commented 2 years ago

Miten muuten jos on styles-attribuutti useassa preamblessa, niin summautuvatko ne missä järjestyksessä vai overridaavat toisiaan.

Tällä hetkellä taidetaan ylikirjoittaa.

Niistä ehkä pitäisi muodostua yhteinen styles-lista?

Joo, mutta menee vähän tämän MR:n ohi, koska pitäisi katsoa ja muokata mergetyslogiikka. Laittaisin mieluummin omaan korttiin ja MRään.

dezhidki commented 2 years ago

Minusta välttämättä automaattisesti preamblejen ei tarvitse tule muuta kuin siletä templates/preambles-hakemistosta. Sitte on eri juttu jos annetaan ihan absoluuttinen yhtä dokun nimeä vastaava polku

Joo, tuo siis oli alkuperäisessä kortissa. Sinänsä preamblet eivät ole dokumenttipohjat, joten siinä mielessä oma preambles-kansio on ehkä järkevä mielestäni. Se saattaa olla myös perustelu sille, miksi kortissa oli tuo ehdotus eikä templates/preambles.

dezhidki commented 2 years ago

Onkos tuolla esimerkkejä missä on annettu niitä absoluuttisia preambleja?
Minusta tuolla kaikki:
https://timdevs01-6.it.jyu.fi/view/users/test-user-1
ovat implisiittisiä.

Joo, nuo ovat minun bugitestit. Sinne voisi tosiaankin laittaa joku testi, jossa on dokumentti + absoluuttinen preamble (tai sekoitus).

dezhidki commented 2 years ago

In GitLab by @vesal on Jan 21, 2022, 11:37

Joo, mutta menee vähän tämän MR:n ohi, koska pitäisi katsoa ja muokata mergetyslogiikka. Laittaisin mieluummin omaan korttiin ja MRään.

Joo, käy... Kuitenkin sivuaa tätä preamble-logiikkaa niin tuli tässä mieleen

dezhidki commented 2 years ago

In GitLab by @vesal on Jan 21, 2022, 11:40

Joo, tuo siis oli alkuperäisessä kortissa. Sinänsä preamblet eivät ole dokumenttipohjat, joten siinä mielessä oma preambles-kansio on ehkä järkevä mielestäni. Se saattaa olla myös perustelu sille, miksi kortissa oli tuo ehdotus eikä templates/preambles.

No käy se tietysti noinkin. Olemassa oleva määrä preambleja on niin iso, että siitä ei voine ihan heti luopua, mutta toki ohjeistuksen voi vaihtaa tuohon toiseen tapaan ja sitten kun saadaan valmiita luotavia dokuja, niin preamble luodaan tuonne. Ideana alunperin on varmaan ollut että käyttäjän hakemistoa ei kuormitettaisi liian isolla määrällä "turhia" nimiä. Nyt en muista edes minne velpit menevät, menevätkö nekin templates?

dezhidki commented 2 years ago

In GitLab by @habca on Jan 27, 2022, 17:38

Commented on timApp/document/docinfo.py line 163

changed this line in version 2 of the diff

dezhidki commented 2 years ago

In GitLab by @habca on Jan 27, 2022, 17:38

Commented on timApp/tests/server/test_preamble.py line 325

changed this line in version 2 of the diff

dezhidki commented 2 years ago

In GitLab by @habca on Jan 27, 2022, 17:38

Commented on timApp/tests/server/test_preamble.py line 343

changed this line in version 2 of the diff

dezhidki commented 2 years ago

In GitLab by @habca on Jan 27, 2022, 17:38

added 23 commits

Compare with previous version

dezhidki commented 2 years ago

In GitLab by @habca on Jan 27, 2022, 18:07

Commented on timApp/document/docinfo.py line 130

Ohjeiden mukaan toteutettiin ja voi käyttää muissa vastaavissa tilanteissa.

dezhidki commented 2 years ago

In GitLab by @habca on Jan 27, 2022, 18:08

Commented on timApp/document/docinfo.py line 163

Tämäkin korjattiin.

dezhidki commented 2 years ago

In GitLab by @habca on Jan 27, 2022, 18:14

Commented on timApp/document/docinfo.py line 160

Toteutin parhaan mukaan kohdissa joihin äkkiseltään soveltui. Testeissä assert-lause varoitti hakasulkeiden käytöstä, niin sinne laitoin list-versiot.

dezhidki commented 2 years ago

In GitLab by @habca on Jan 27, 2022, 18:35

Voisi olla periaatteessa niin, että ladattavien preamble-listasta poistetaan aina nykyinen dokumentti itse.

Toteutustavaksi valittiin viitteen poistaminen ladattavien preamble-dokumenttien listasta. Tilanne korjaantui testipalvelimella ja sille oli helppoa kirjoittaa testitapaus.

Sinne voisi tosiaankin laittaa joku testi, jossa on dokumentti + absoluuttinen preamble (tai sekoitus).

Testipalvelimella on kanssa dokumentti + absoluuttinen preamble, mitä aiemmin kaivattiin.

automaattisesti preamblejen ei tarvitse tule muuta kuin sieltä templates/preambles-hakemistosta

Templates-hakemiston oletuksen ehtii vielä palauttamaan ennalleen, jos pitää alkuperäisen toteutustavan. Syynä miksi ylipäänsä vaihdettiin oli se, että absoluuttiset preamblet löytyvät samasta myös pelkällä nimellä etsittynä, jos saman polun varrella.

dezhidki commented 2 years ago

In GitLab by @habca on Jan 27, 2022, 22:14

added 4 commits

Compare with previous version

dezhidki commented 2 years ago

In GitLab by @habca on Jan 27, 2022, 23:03

Commented on timApp/document/docinfo.py line 144

Templates-hakemiston vaatimus palautettiin takaisin, koska sitä ei oikeastaan edes pyydetty. Lähdekoodia sai yksinkertaisemmaksi, mikä on hyvä asia.

dezhidki commented 2 years ago

In GitLab by @vesal on Jan 28, 2022, 07:29

mites tuohon saa myös sen normaalin?

dezhidki commented 2 years ago

In GitLab by @habca on Jan 28, 2022, 08:59

mites tuohon saa myös sen normaalin?

Normaali tapa on tällä hetkellä ainoa. Mitäpä sitä toimivaa muuttamaan.

dezhidki commented 2 years ago

Noita ja en ehkä käyttäisi kommentteisa. GitLabikin varoittaa niistä näköjään:

image

dezhidki commented 2 years ago

Laitoin yhden pienen kohdan, muuten puolestani näyttää hyvältä. Testaillen vielä hieman devsissa ennen kuin otan tuotantoon ellei tule muita huomioita. Siinä on nyt tuo yksi kohta ylempänä, josta Vesa kyseli.

dezhidki commented 2 years ago

In GitLab by @vesal on Jan 28, 2022, 09:05

siis jos haluan yhtäaikaa sekä absoluuttisen että normaalin. Yritin tuonne esimerkkidokuun.

Köyttötapaus: Gradu joka hakee gradun asetukset yhdestä yhteisestä paikasta ja sitten hakemistokohtaiset tekijön omat makrot normaalisti.

dezhidki commented 2 years ago

Joo, ihan hyvä näin.

dezhidki commented 2 years ago

Tarkoitat siis varmaan tätä?

https://timdevs01-6.it.jyu.fi/view/users/test-user-1/test-absolute-preambles/document1

Siinä oli asetuksissa väärin kirjoittettu preamble eikä preambles

dezhidki commented 2 years ago

Eikun korjaus, pitääkin olla preamble. Mutta siis preamble ei ole lista, vaan pilkulla erotettu merkkijono.

dezhidki commented 2 years ago

In GitLab by @vesal on Jan 28, 2022, 09:41

https://timdevs01-6.it.jyu.fi/view/users/test-user-1/test-absolute-preambles/document1

Siinä oli asetuksissa väärin kirjoittettu preamble eikä preambles

Mielesätni mulla oli tuollainenkin kokeilu:

https://timdevs01-6.it.jyu.fi/download/72/2/1

Mutta onko nyt siis sekä preamble että premables käytössä? Toiselle muoto jossa samalle riville ja toiselle missä eri riville.

Tuo preamble jos asetetaan, niin entinen versio oli sellainen, jossa se kumosi kokonaan prustavan. Onko se edelleen käytössä (tai moniko dokumentti nojaa tuohon?, mulla ainakin demo0)? Vai onko niin, että jos tuossa on absoluuttinen polku, niin silloin se ei kumoa perustapaa? Kumpiko otetaan silloin ensin?

Jos vanhoja käyttätapauksia ei ole mahdottomasti, voisi nyt yrittää miettiä loogisimman.

Vesa

dezhidki commented 2 years ago

Tarkensin asian ketjun ylemmässä viestissä. Eli siis preamblen toimintaa ei ole muutettu, vaan se on edelleen pilkulla erotettu merkkijono eikä YAML-lista.

Puolestani preamble voisi olla ihan kunnon listakin, eli että voisi sallia syntaksi

preamble:
  - a
  - b

Harri voi vaikka päättää, tehtäisiinkö tähän MRään, vai tekisikö tuon omassa MRssä. Korjasin kuitenkin tuon mallidokun oikean syntaksin mukaan.

dezhidki commented 2 years ago

In GitLab by @vesal on Jan 28, 2022, 09:54

preamble:

  • a
  • b

Harri voi vaikka päättää, tehtäisiinkö tähän MRään, vai tekisikö tuon omassa MRssä. Korjasin kuitenkin tuon mallidokun oikean syntaksin mukaan.

Joo, jos saisi etsittyä onko tuollaisi pilkulla eroteltuja yhtään, niin minusta voisi vaihtaa tuohon listaan. Tai sitten niinkuin hetken luulin että on molemmat

preamble: yksirivi preambles:

ja vain jompaa kupaa pitää käyttää ja preamble-muoto on sellainen, että se kumoaa oletuksen, sallii vain yhden alkion ja on deprecated. Ohjeet vain preambles mukaisesti?

dezhidki commented 2 years ago

In GitLab by @habca on Jan 28, 2022, 11:26

Ehdotan, että laitetaan nykyinen MR-tuotantoon ja keskeytetään preamble:-asetuksen kehittäminen, koska

tärkeimmät ja kiireelliset vaatimukset toteutettiin. Muille vaatimuksille ehdotan tehtäväksi täysin erillisen ja nykyisestä riippumattoman kortin uudelle preambles:-asetukselle, joka sisältää

  1. YAML-listan
  2. järjestyksen
  3. suhteellisen polun ilman perintää
  4. absoluuttisen polun ilman perintää
  5. alihakemistot mahdollistavan globbauksen
  6. yksittäisen preamblen kieltäminen (YAML-lista?)
dezhidki commented 2 years ago

Joo, ihan hyvä puolestani. Laitatko tämän uuden korttiin? Sen alkuperäisen #1103 voi sitten korvata tällä uudella jatkokehityskortilla mergeyksen jälkeen.

dezhidki commented 2 years ago

In GitLab by @habca on Jan 28, 2022, 11:41

Commented on timApp/document/docinfo.py line 127

changed this line in version 5 of the diff

dezhidki commented 2 years ago

In GitLab by @habca on Jan 28, 2022, 11:41

added 1 commit

Compare with previous version

dezhidki commented 2 years ago

In GitLab by @habca on Jan 28, 2022, 11:43

Commented on timApp/document/docinfo.py line 127

Ok, korvattu lainausmerkeillä.

dezhidki commented 2 years ago

resolved all threads

dezhidki commented 2 years ago

approved this merge request

dezhidki commented 2 years ago

enabled an automatic merge when the pipeline for 1a1462f37ff557e20e516db94a691b17cc328cf6 succeeds

dezhidki commented 2 years ago

unapproved this merge request

dezhidki commented 2 years ago

Otankin vielä hetkeksi pois Approve, testaan vielä timdevs02:ssa.

dezhidki commented 2 years ago

canceled the automatic merge

dezhidki commented 2 years ago

approved this merge request

dezhidki commented 2 years ago

enabled an automatic merge when the pipeline for 1a1462f37ff557e20e516db94a691b17cc328cf6 succeeds

dezhidki commented 2 years ago

canceled the automatic merge

dezhidki commented 2 years ago

enabled an automatic merge when the pipeline for 1a1462f37ff557e20e516db94a691b17cc328cf6 succeeds