Conversation
This includes some commented code that will be removed in the following commits just so that I have it saved somewhere in the history as a previous attempt in case we want to go back to it.
joelostblom
force-pushed
the
feat/beautify-cli-help
branch
from
31e2dec to
c9251ff
Compare
.coveragerc
Outdated
| @@ -0,0 +1,2 @@ | |||
| [run] | |||
| omit = src/seedcase_flower/internals.py | |||
There was a problem hiding this comment.
This excludes our private code from the coverage percentage. I remember we mentioned before that we only test public facing code (and implicitly the private code as it is used in the public facing code). I don't know if we want it formal through a change like this but I'm including it as a suggestion since this PR fails coverage without it (could be made its own PR).
There was a problem hiding this comment.
I'm not sure I understand. Is that because the code you're adding isn't used when running via tests, so doesn't get picked up by the coverage?
There was a problem hiding this comment.
Yes, I'm unsure how to test this since there is no public function calling this code. It is only used during the initialization of app, should we create a test for app --help which compares the output help string to a fixed test case? It seems a bit wonky, but that the public facing component is correct formatting of the help message.
It would be easier to test the private function directly, but then there could be a gap to what we think is the correct behavior and what actually makes the help message look the way it should.
There was a problem hiding this comment.
Hmmm, I think for now, it's fine to keep it in the coverage percent. It truthfully shows that not all code has been covered, which is true! 😛 So you can delete this file
There was a problem hiding this comment.
Sounds good, removed!
lwjohnst86
changed the title
lwjohnst86
left a comment
lwjohnst86
left a comment
There was a problem hiding this comment.
Super nice! Just some very small suggestions and a question
.coveragerc
Outdated
| @@ -0,0 +1,2 @@ | |||
| [run] | |||
| omit = src/seedcase_flower/internals.py | |||
There was a problem hiding this comment.
I'm not sure I understand. Is that because the code you're adding isn't used when running via tests, so doesn't get picked up by the coverage?
github-project-automation
bot
moved this from In Review
to In Progress
in Iteration planning
.coveragerc
Outdated
| @@ -0,0 +1,2 @@ | |||
| [run] | |||
| omit = src/seedcase_flower/internals.py | |||
There was a problem hiding this comment.
Hmmm, I think for now, it's fine to keep it in the coverage percent. It truthfully shows that not all code has been covered, which is true! 😛 So you can delete this file
github-project-automation
bot
moved this from In Review
to In Progress
in Iteration planning
Co-authored-by: Luke W. Johnston <lwjohnst86@users.noreply.github.com>
signekb
left a comment
signekb
left a comment
There was a problem hiding this comment.
A comment about a comment 💭 :
github-project-automation
bot
moved this from In Review
to In Progress
in Iteration planning
lwjohnst86
left a comment
lwjohnst86
left a comment
There was a problem hiding this comment.
Wow, coverage is at 89.something percent and we've set it at a threshold of 90 haha
Related to that, you can do some Unit Testing of the app: https://cyclopts.readthedocs.io/en/stable/cookbook/unit_testing.html#unit-testing
Can you try to implement it? Would be useful for us to have that infrastructure/code to know how to test the CLI.
github-project-automation
bot
moved this from In Review
to In Progress
in Iteration planning
|
Great find! I agree it would be great to add tests for the App interface in general. Do you want them in this PR or a separate one to keep this one smaller (and since it will also include tests for CLI code that is not new additions in this PR but existed since previously such as testing that the config file is being read)? |
lwjohnst86
left a comment
lwjohnst86
left a comment
There was a problem hiding this comment.
Yea if you could add the tests that would be great 👍
github-project-automation
bot
moved this from In Review
to In Progress
in Iteration planning
|
I think you'll be able to keep the tests mostly specific to this PR, if my reading of the docs is accurate. |
|
I will add test, but if we can review and merge #155 first then it will be easier as I can base it on that general app testing skeleton and just add tests for the additional functionality here. |
This is a first implementation of the tests we can use to check that the CLI is behaving correctly. I think this covers most of what we currently have in main, at least we reach 💯 % coverage! Merging this before some of the existing PRs e.g. #140 and #152 will make it easier to add new tests specifically for the functionality added there instead of convoluting it with testing the general functionality of the app which is tested here instead. I tried to follow the cyclopts test docs as closely as possible, but let me know if you think something is missing. I also experimented with using the Copilot CLI for the first time to help me get started and understanding how to write the tests. It was quite useful, particularly to get started, but I did have to delete about half of its suggestions because there was a lot of redundancy. Thought I would mention it in case someone catches something odd in here so I can blame it and not me =p Needs a thorough review. ## Checklist - [x] Ran `just run-all` --------- Co-authored-by: Luke W. Johnston <lwjohnst86@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
4 participants
Description
Changes the help format from
to
Details in #138
Closes #138
Needs a a thorough review.
Checklist
just run-all