cv-rs

cv-rs copied to clipboard

Following up on issue #105. Does it look sane to you? Please do not pull yet. I want to add more of its methods.

So far so good.

P.S. Hint of the day: if you aren't ready yet start PR name with WIP: 🙂

Sorry for imposing. Got advice on writing tests? @Pzixel

just write them. I'm not sure, there is not special advices here. Setup-act-assert, classic loop

Biggest patch I had ever wrote. What do you think, @Pzixel? I wrote some tests, too!

Looks great.

I can't look at it right now, but I gonna come back later.

P.S. Check for formatting, because some of changes are just format ones, and CI server won't allow it ;)

---

Why so much force pushes? I'l squash all commits into one when merging. It's much safer and more convenient. You can have buzillion of commits and they all becomes one.

Looks great.

I can't look at it right now, but I gonna come back later.

Thank you.

P.S. Check for formatting, because some of changes are just format ones, and CI server won't allow it ;)

I didn't realize that. I will look and see what's what tomorrow.

Why so much force pushes? I'l squash all commits into one when merging. It's much safer and more convenient. You can have buzillion of commits and they all becomes one.

The following is one part thinking out loud and one part asking for advice:

I will create another branch to work on, then merge with this one. I don't know if I should keep, squash, or fixup all the tiny bug fixing commits along the way. Maybe I should just let it accumulate and decide right at the end. I will commit fixup ones with message "f".

Just keep pushing commits as they go. There is no shame having ~30-40 commits with no sane message since they anyway will go away. Example from my private repo

becomes

When merging

I do not understand the appveyor test failure. Locally it is running fine. My OpenCV version is 3.4.5.

Got any ideas on how to find the reason it had failed?

Look at output:

    Running `C:\projects\cv-rs\target\debug\deps\test_video_analysis-3a0f73e01162fd28.exe`
running 6 tests
test knn_tests::test_create_background_subtractor_knn_apply ... ok
test knn_tests::test_create_background_subtractor_knn_default ... ok
test mog2_tests::test_create_background_subtractor_mog2_apply ... ok
error: test failed, to rerun pass '--test test_video_analysis'
Command exited with code -1073741819

Considering it's only failing on Windows it's some windows-specific problem. Either it's something windows can't run (then it should be hidden via cfg) or there is some bug linux is not affected of.

Should I add something like #[cfg(not(windows))]? Wouldn't that mask the missing coverage?

Firstly you should figure out what's wrong on Windows. It's like 99% chance that you're doing something wrong: e.g. if you do Command::new("curl") to make an HTTP request it will work in linux, but won't on windows.

The furthest I can go in making it work on Windows is generalizing the path. I don't have a Windows machine I can use.

I wouldn't be able to run it on windows till next week.

You probably could check it on windows VM.

In worst case we have to configure it out via cfg and open "up to grabs" issue, but I hope it won't be required.

I can follow through on this PR after #117 is merged.