-
Notifications
You must be signed in to change notification settings - Fork 41
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
WIP: Add Background Subtractors. #106
base: master
Are you sure you want to change the base?
WIP: Add Background Subtractors. #106
Conversation
So far so good. P.S. Hint of the day: if you aren't ready yet start PR name with |
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 |
17b287c
to
33a8773
Compare
3332f9c
to
c49b466
Compare
c49b466
to
5a8e12c
Compare
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. |
Thank you.
I didn't realize that. I will look and see what's what tomorrow.
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 do not understand the appveyor test failure. Got any ideas on how to find the reason it had failed? |
Look at output:
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 |
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 |
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 |
I can follow through on this PR after #117 is merged. |
Following up on issue #105.
Does it look sane to you?
Please do not pull yet. I want to add more of its methods.