Add command line option "--noreorder" to "add" command #625
Conversation
Signed-off-by: Florian Wühr <[email protected]>
There was a problem hiding this comment.
Hi @fwuehr1995 and thanks for the PR!
I see what you mean, and I agree that reordering a document on add might be a bad idea.
However, I believe we should discuss this issue before approving this, because if we want to remove the reorder as a default, this PR will be unnecessary.
Also, there is some functionality missing here. Your PR only adds the argument, it does not actually instruct the add_item function in document.py to not reorder.
There are also no tests covering the committed code.
|
Hi @neerdoc, thanks for your comments. Nevertheless, if it would make sense to get rid of the reordering by default, I would agree that this PR is unnecessary. Besides that, I am not sure if I get your concerns regarding the implementation in document.py right, I tested my implementation and it fully works. By not specifying the argument at the "doorstop add" call, the reordering parameter of the add_item function stays at the default value which is True. If the argument is specified though, this changes to false due to the assignment of "reorder=args.noreorder" in the actual function call. Regarding missing tests, I am happy to provide them, but if we decided to remove the automatic reordering anyway my PR would be obsolete. So the main question for me: How to proceed with your suggestion of removing the automatic reordering in general ? |
|
Ah, you are correct! I did not notice that all args are added automatically to the "add_item" call! @jacebrowning What do you think? Should "reorder" really be the default when adding items to a document? IMHO I believe that it introduces an unnecessary overhead and that the reordering should only be done when requested by the user. |
|
@neerdoc Can you clarify what you mean by "extra overhead"? Running the |
|
@jacebrowning I am using the doorstop add command in a shell script for multiple times in a row to add to the same file, but I notice that the execution time of doorstop add is significantly increasing the bigger the file to add to gets. Without the reordering every time we can decrease the execution time for each "doorstop add" to about 60 percent of what it was before. So my intention is basically to do the reordering once after all necessary items have been added and not everytime that something gets added. This just for your information to classify the idea. |
|
I would assume that the cost of the reordering is at least |
|
My assumption is that the scenario of adding lots of items at once is far less common than adding a single item. If that's true, then I think we should keep the default behavior but add this option to allow the default behavior to be skipped. |
|
Alright, let's go with that then! @fwuehr1995 if you can add tests for this functionality it should be an easy approve! |
Signed-off-by: Florian Wühr <[email protected]>
There was a problem hiding this comment.
While this test most likely will get complete coverage of the new code, I would prefer adding two tests.
- A test that adds a new item without using the new flag and thus checks that an automatic reorder is done correctly be reading item levels (I don't think there is a proper test for this at the moment).
- The same test with the
--noreorderflag set to make sure the document is not reordered.
There was a problem hiding this comment.
@neerdoc isn´t this actually covered by the tests of document.py in "test_document.py" ? At least it should be from my point of view.
I am just curious because I basically didn´t change any functionality in the methods of document.py. I am just using the already existing mechanism to reorder or not to reorder that the parameter "reorder" of the function "add_item" has, which was there before I started working on the code.
What I am trying to avoid is to write tests now for code which already existed in the first place and was not modified through my changes.
I didn´t change the default behavior and the changes I introduced would be covered by the test I added.
So honestly I don´t feel the need to provide these tests in order to safeguard my PR because I didn´t modify anything in these circumstances.
There was a problem hiding this comment.
Your test will cover your change, yes. But what does it actually test? It verifies only that the main function in the file doorstop/cli/main.py does not throw an error when you add the flag --noreorder.
It does not test that the add command to the CLI prevents the reordering of the document. So from that perspective, it is not a functional test, it's simply a call to get test coverage.
If there is another PR in the future that makes a change to how the reorder flag works, say renaming the function parameter, then this test will still pass even though the flag does not work anymore.
The aim for a test should always be to test the functionality, not to reach a coverage measurement.
There was a problem hiding this comment.
Fair enough, I get your point which is definitely valid.
To be honest I am not so experienced with writing proper tests for Python projects and in particular in the context of the doorstop, which is why I wanted to avoid diving in so deep. But of course your concerns are valid, so I will have to take some time for that.
I will request another review whenever I have improved tests available.
There was a problem hiding this comment.
@neerdoc I have now added improved tests which definitely make sure that the add_item function is being called with the right parameters. The actual reordering function seems to be pretty well tested already in "test_document.py" IMHO.
I hope this is more like it shall be.
There was a problem hiding this comment.
@neerdoc I don´t want to annoy you but since you offered help for the tests, I would like to reach out to you again.
There was a problem hiding this comment.
@fwuehr1995 No problem, did you read my (rather long) reply above?
There was a problem hiding this comment.
@fwuehr1995 Sorry, it seems I was stuck in "Reviewing", so I don't believe you have seen the comment?
There was a problem hiding this comment.
No, I haven´t. Thanks for that great support, I will dive into it and provide an update next week. Cheers !
There was a problem hiding this comment.
I have decided to take the easy way out, which fits best for me at the moment.
I will try to get more deeply involved in the future since this what you correctly said "Open Source" is all about.
Signed-off-by: Florian Wühr <[email protected]>
Signed-off-by: Florian Wühr <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #625 +/- ##
========================================
Coverage 96.25% 96.25%
========================================
Files 41 41
Lines 5159 5160 +1
Branches 1214 1214
========================================
+ Hits 4966 4967 +1
Misses 121 121
Partials 72 72 ☔ View full report in Codecov by Sentry. |
|
@neerdoc thanks for the approval, much appreciated. Here is the output of my "make check" run: wuehr@fwuehr-thinkpadt14gen4:~/Projects/git/doorstop$ make check poetry run mypy doorstop --config-file=.mypy.ini The above exception was the direct cause of the following exception: Traceback (most recent call last): |
|
Anybody out there that can tell me what is going wrong with the checks ? |
|
@fwuehr1995 I've had a look now and there are two different issues.
|
|
@neerdoc got it, there was some formatting missing. I pushed the changes generated by "make check", I hope we should be good to go now |
|
You still have a problem with the Windows file release issue. Please rebase to the latest |
In order to have the possibility to run the "doorstop add" command multiple times (e.g. in a shell script) without having to reorder the entire file each time the "add" command is invoked, it would be great to have the optional parameter to disable reordering.