From 638693ba44d220cf80d55ffe621cb8277b218227 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20N=C3=BCtzi?= Date: Tue, 11 Jun 2024 14:04:31 +0200 Subject: [PATCH 1/4] docs: Add review comments --- docs/review-comments.md | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 docs/review-comments.md diff --git a/docs/review-comments.md b/docs/review-comments.md new file mode 100644 index 00000000..cddaa60c --- /dev/null +++ b/docs/review-comments.md @@ -0,0 +1,39 @@ +# Review Comments 💁 + +## Project Structure 🚀 + +The project is in good structure already. I like it! +You did a good job. File-naming is consistent, nice small kebab-case folder names. Already in good shape with BPA Repository Guidelines I think, (also there is much more I like to see, later =)). Overall continue what I see and beeing clean and precise etc. ❤️ + +- `has_sample` variables in `.zarr` files implies `bool` values. Maybe consider + `sample(s)`. Check also for other use cases of this pattern. + +- Maybe add a `.modos` extension to `ZARR` root folder (representing the file + format). This gives the whole thing a much nicer semantic meaning. Yes its a file-format =). + +- Maybe put all top-level files `.bcf` etc. in `ZARR` files into `root/assets` (e.g. `root=/data/ex`). To make the toplevel of the file `.modos` clean. =) + +- I would introduce the following folder structure on top-level + to have it even more clean. + + - move `modos` -> `src/modos` : The `modos` source code. + - move `data` -> `examples/` + - create `tools` : Tools and scripts needed in the future for this repository. Tooling around certain development aspects. + - move `deploy` -> `tools/deployment`: Move the folder maybe there. + - move `nix` -> `tools/nix`. + + **or** if you want a more mono-repository structure: + + - `components/modos`: the python component providing the python source code, its a folder which acts as a full python project. + + - `components/modos-backend` : the stuff which is needed to deploy the `backend`, e.g. `htsget` etc... + + This mono-repo structure makes it easier to add more stuff later, if we need another micro-service or a database etc. + But maybe overkill for now! + +- Provide a `.devcontainer` such that `code` users can simply run everything inside this container. I can make a short PR on this providing the minimal setup (ubuntu with nix installed, nice shell, etc). To make it more proper: + _We need a `python` dev-container maintained centrally to pull this off properly. I can provide all this but needs discussion first in BPA and SDSC in general how you did things etc._ + +## Code Review 🐊 + +TODO: ... From a99bbf9afe7081b0bf7203341999540b4a717d1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20N=C3=BCtzi?= Date: Tue, 11 Jun 2024 14:07:24 +0200 Subject: [PATCH 2/4] fix: Add some more stuff --- docs/review-comments.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/review-comments.md b/docs/review-comments.md index cddaa60c..21476d6f 100644 --- a/docs/review-comments.md +++ b/docs/review-comments.md @@ -34,6 +34,8 @@ You did a good job. File-naming is consistent, nice small kebab-case folder name - Provide a `.devcontainer` such that `code` users can simply run everything inside this container. I can make a short PR on this providing the minimal setup (ubuntu with nix installed, nice shell, etc). To make it more proper: _We need a `python` dev-container maintained centrally to pull this off properly. I can provide all this but needs discussion first in BPA and SDSC in general how you did things etc._ +- Maybe do not write `.bat` files (reasons on 1-to-1 😃), maybe `make.bat` was autogenerated or so. But Windows users are much encouraged to use `git-bash.exe` (`bash.exe`) and everything in this repository should work out of the box. Or they should use the `.devcontainer` with `code`. This should be maybe \*nix only. + ## Code Review 🐊 TODO: ... From fc150668cece3ef16a3b2799e734b6a2a00bdbe7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20N=C3=BCtzi?= Date: Tue, 11 Jun 2024 19:47:40 +0200 Subject: [PATCH 3/4] fix: Add some stuff --- docs/review-comments.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/review-comments.md b/docs/review-comments.md index 21476d6f..ad66d658 100644 --- a/docs/review-comments.md +++ b/docs/review-comments.md @@ -36,6 +36,9 @@ You did a good job. File-naming is consistent, nice small kebab-case folder name - Maybe do not write `.bat` files (reasons on 1-to-1 😃), maybe `make.bat` was autogenerated or so. But Windows users are much encouraged to use `git-bash.exe` (`bash.exe`) and everything in this repository should work out of the box. Or they should use the `.devcontainer` with `code`. This should be maybe \*nix only. +- Write all titles in markdown files capitalized (correct English). I am somewhat unsure with stuff like `Welcome to the modos-api documentation`. + But I think consistency matters more. (?) + ## Code Review 🐊 TODO: ... From 84ea43cafb3ea0ff8043050547cf61b43e435cdb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20N=C3=BCtzi?= Date: Wed, 12 Jun 2024 10:42:05 +0200 Subject: [PATCH 4/4] fix: Some more stuff --- docs/review-comments.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/review-comments.md b/docs/review-comments.md index ad66d658..3cb6bb7a 100644 --- a/docs/review-comments.md +++ b/docs/review-comments.md @@ -5,6 +5,8 @@ The project is in good structure already. I like it! You did a good job. File-naming is consistent, nice small kebab-case folder names. Already in good shape with BPA Repository Guidelines I think, (also there is much more I like to see, later =)). Overall continue what I see and beeing clean and precise etc. ❤️ +All below items are equally important until prioritization: + - `has_sample` variables in `.zarr` files implies `bool` values. Maybe consider `sample(s)`. Check also for other use cases of this pattern. @@ -39,6 +41,8 @@ You did a good job. File-naming is consistent, nice small kebab-case folder name - Write all titles in markdown files capitalized (correct English). I am somewhat unsure with stuff like `Welcome to the modos-api documentation`. But I think consistency matters more. (?) +- Change `Makefile` to `justfile`: Its a better and nicer command runner. `make` is really old and has some arcane workings I want nobody to expose to =). + ## Code Review 🐊 TODO: ...