Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KANBAN-504 seq retrieval #2

Merged
merged 32 commits into from
Mar 14, 2024
Merged

KANBAN-504 seq retrieval #2

merged 32 commits into from
Mar 14, 2024

Conversation

mluypaert
Copy link
Member

Hey @christabone, would you mind reviewing this PR?
Not as much whether the code would be functional (I assured that through testing) but more if the code itself is readable to you, whether the repository folder structure is sensible to you, whether the development, typing and documentation guidelines I've written in the README make sense etc.
The goal is to make this repository accessible to other (AGR) developers should they wish to contribute or need to take over future development. So your review should essentially boil down to the question "If I were to hand over this repository to you to continue development, would you be comfortable to do so with the current state of files, guidelines, rules and checks? And if not, what's missing?". I still need to add unit testing (and add that to the PR validation) so I'm aware of that, but otherwise I think everything in this PR should be ready to be worked on by anyone with the relevant subject knowledge.

This enables relevant IDE tooltip hints in VS Code (and presumably other IDEs) and thus more efficient coding.
@valearna
Copy link

@mluypaert I don't know much about this project and I haven't even tried to understand the details of each function, but from a very high-level perspective it looks good to me, considering how readable and organized the code is and how you structured config files and documentation.

@sweng66
Copy link

sweng66 commented Mar 14, 2024

Just checked out the repo and it looks clean and well commented. The seq retrieval code also looks good! Thanks @mluypaert

@christabone
Copy link

@mluypaert This looks really good -- no problems at all with the structure and your comments are great. I think we'd have no problem passing this around between developers if necessary.

@ianlongden
Copy link

In the checks before merging is allowed it would be nice to have flake8 or something of that type that has to pass too.

@mluypaert
Copy link
Member Author

In the checks before merging is allowed it would be nice to have flake8 or something of that type that has to pass too.

@ianlongden Good point, I was planning to potentially add that at some later point, but having had a closer look at flake8 it was easy enough to just enable it straight away, so I've added that as well now. Thanks!

@mluypaert mluypaert merged commit 7df062c into main Mar 14, 2024
4 checks passed
@mluypaert mluypaert deleted the KANBAN-504_seq-retrieval branch March 14, 2024 21:19
@mluypaert mluypaert restored the KANBAN-504_seq-retrieval branch March 14, 2024 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants