Add command tsort
#2984
Add command tsort
#2984
Conversation
@rminnich Good question! I assumed that all dependencies were vendored in this project. Am I mistaken? Anyway, my test needs the |
|
did you do a go mod update? That usually does it for me? |
I did, and apparently it doesn't work. After reverting the vendor update with 48710d2, running This suggests to me that vendoring is mandatory. Furthermore, my IDE doesn't see the Am I missing something obvious? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2984 +/- ##
==========================================
+ Coverage 55.61% 56.03% +0.42%
==========================================
Files 512 517 +5
Lines 31139 31326 +187
==========================================
+ Hits 17319 17555 +236
+ Misses 13820 13771 -49
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
another question, sorry :-)
Can you make a tsort package and have it implement the interface:
https://medium.com/@kdnotes/sort-sort-interface-in-golang-1d263d96956d
rminnich
added
Awaiting author
|
also, I think my interface question was dumb, so nvm. I will test out your PR |
|
I pulled your PR, removed the vendoring, |
@rminnich No worries, I totally understand. I made the same mistake too when first learning about tsort. 😁 |
|
I'm tempted to move the |
|
@rminnich @binjip978 I'm ready for another review when you are! When we're done, I intend to make two signed-off commits, one for the |
|
By the way, I noticed that CIFuzz didn't catch a bug I found towards the end of development, so I was wondering if there's a way to tell it to fuzz |
|
I've squashed many bugs over the last few days, so I'm doubly ready! |
binjip978
added
Awaiting reviewer
|
Dont' forget to git commit --amend -s |
|
Also, we like to maintain the rule that the tests work for any commit. Might be best to squash these many commits? Thanks. |
rminnich
added
Awaiting author
You're very welcome, this was fun to work on!
Okay, on it. 👍
Cool, I was planning to squash and sign the commits sooner or later, I just wasn't sure when you wanted me to do so, so I'm on it. |
jbduncan
force-pushed
the
tsort
branch
3 times, most recently
from
fb27180
to
5c42388
Compare
|
@rminnich @binjip978 I'm ready for another review whenever you are. |
|
I think the code coverage has gone down because, due to the prevalence of maps in this implementation, one or two of the code paths aren't always visited by the tests. I'm not entirely sure what to do about them, so I'm happy to leave it to your judgement. |
This introduces a version of tsort that follows the POSIX tsort specification except for the "Environment Variables" section: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/tsort.html Signed-off-by: Jonathan Bluett-Duncan <jbluettduncan@gmail.com>
|
@rminnich @binjip978, I see that @binjip978 approved things a week ago, so I was wondering if there's anything else I need to do before this can be merged in? I'm aware there was a drop in code coverage at some point, but I don't think it's an issue. I think it's just flaky because the implementation relies on maps heavily, so one or two of the code paths aren't always visited by the tests. |
binjip978
added
Awaiting reviewer
No worries, I updated the label, but if no new comments will follow I will merge it soon. |
|
Woo! Really happy to see this merged in, so thank you for reviewing this and being so receptive, @binjip978 and @rminnich. :) |
Add
cmds/exp/tsort, which aims to implement POSIX tsort (except for "Environment Variables").Currently I'm undecided what to do about cycles, as handling them is tricky and POSIX takes no stance on them.
There are still some remaining TODOs in the source code.
Resolves #2982.