Repo structure cleanup - Githubissues

kraus-s commented 2 years ago

Addressing issue #100 Reference: https://www.youtube.com/watch?v=VYycQTm2HrM

kraus-s commented 2 years ago

Must have overlooked the docs and makefile. Will fix that later today. Thanks for pointing that out!

I also thought about further dividing the lib folder, but we can always address that later.

Concerning the pages folder: I have not yet tested that.

On Fri, 8 Jul 2022, 16:05 Balduin Landolt, @.***> wrote:

@.**** requested changes on this pull request.

mostly looks good, the code seems very tidy now! But no wonder, with such an instructive reference video... :) (I think at some point we should even split up the lib folder, but that can wait.)

Two things I don't like:

1.

the makefile is gone, so now the test scaffolding doesn't work anymore. 2.

the docs folder and all its contents are gone too, which was also scaffolding for future work.

I think these changes should be reverted.

Just out of curiosity: did you test what streamlit makes out of the pages folder?

— Reply to this email directly, view it on GitHub https://github.com/arbeitsgruppe-digitale-altnordistik/Sammlung-Toole/pull/104#pullrequestreview-1033042532, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANZXABDOHWTJJIIKC7ZWWULVTA7S7ANCNFSM53BCNPIA . You are receiving this because you authored the thread.Message ID: <arbeitsgruppe-digitale-altnordistik/Sammlung-Toole/pull/104/review/1033042532 @github.com>

kraus-s commented 2 years ago

Most recent commit is now again working and addresses what I overlooked.

Two things: I noticed that for some odd reason the runner script was not working anymore (now that I think of it, not sure if it ever did), because pipenv is not on my path (can't because I don't have local admin...). Secondly, it now throws an error I can't figure out (might be the time of day, though): src\lib\groups.py", line 66, in from_cache g = pickle.load(f) ModuleNotFoundError: No module named 'util'`

Also, concerning the pages: Streamlit makes a sort of side bar menu of its own from the files it finds in the pages folder. Not sure I like it, though.

kraus-s commented 2 years ago

Broke the docs test. Will fix tomorrow.

On Fri, 8 Jul 2022, 17:14 Sven Kraus, @.***> wrote:

Must have overlooked the docs and makefile. Will fix that later today. Thanks for pointing that out!

I also thought about further dividing the lib folder, but we can always address that later.

Concerning the pages folder: I have not yet tested that.

On Fri, 8 Jul 2022, 16:05 Balduin Landolt, @.***> wrote:

@.**** requested changes on this pull request.

mostly looks good, the code seems very tidy now! But no wonder, with such an instructive reference video... :) (I think at some point we should even split up the lib folder, but that can wait.)

Two things I don't like:

1.

the makefile is gone, so now the test scaffolding doesn't work anymore. 2.

the docs folder and all its contents are gone too, which was also scaffolding for future work.

I think these changes should be reverted.

Just out of curiosity: did you test what streamlit makes out of the pages folder?

— Reply to this email directly, view it on GitHub https://github.com/arbeitsgruppe-digitale-altnordistik/Sammlung-Toole/pull/104#pullrequestreview-1033042532, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANZXABDOHWTJJIIKC7ZWWULVTA7S7ANCNFSM53BCNPIA . You are receiving this because you authored the thread.Message ID: <arbeitsgruppe-digitale-altnordistik/Sammlung-Toole/pull/104/review/1033042532 @github.com>

BalduinLandolt commented 2 years ago

let me know if I should look into something!

Some thoughts:

the docs test isn't actually a test, it just tries to build the docs to see if it fails. So the mkdocs stuff shouldn't live in the tests folder: mkdocs.yml living in the root folder probably fixes that already
the issue with loading the pickles should go away if you just delete the files created with the old structure, so that it recreates them
if we don't like the stramlit pages (we should look into that in a separate PR) then you probably just rename the folder to pgs for now, so that it behaves like it used to. ^^

kraus-s commented 2 years ago

Fixed tests and the rest. Imho ready for pull?

arbeitsgruppe-digitale-altnordistik / Sammlung-Toole

Repo structure cleanup #104