Almost ready just a minor bug needa to be resolved
Pranam Lashkari
@lpranam
@mloskot just created a new PR for the same
Mateusz Łoskot
@mloskot
Approved. Thanks for speedy PR!
Pranam Lashkari
@lpranam
:)
Olzhas Zhumabek
@simmplecoder
should I hide my code under detail too, since it depends on the convolve_2d?
Olzhas Zhumabek
@simmplecoder
it is weird that in my PR the public interface returns class from detail
Pranam Lashkari
@lpranam
@simmplecoder your argument makes sense to me..
Mateusz Łoskot
@mloskot
@simmplecoder It all depends on what "since it depends on class from detail" mean, I think
Since it is about returning, then you and @lpranam rightly observe it smells
But, it also can be fine and indicate user that type is internal and avoid using the type in own interfaces
While using instances of the type in user's code is perfectly fine
Mateusz Łoskot
@mloskot
After all, auto x = foo() ignores the type, user does not have to care type of x lives in detail
So, I'd not rush with hiding your code
I'd rather like to hear what others think, what @stefanseefeld thinks
Mateusz Łoskot
@mloskot
@simmplecoder if you prefer to open an issue referenced to my PR hiding the stuff, by all means, feel free to do so, mentioning me, @stefanseefeld and whoever you'd like to consult. Then let's discuss it there (you know here things are easily overlooked, Stefan is not here all the time)
@mloskot, feel free to ping me if you see anything suspicious in my PR. I believe the tests should pass now
Mateusz Łoskot
@mloskot
@simmplecoder OK. The only thing we are missing now is review from @stefanseefeld , I think
Olzhas Zhumabek
@simmplecoder
yeah, the likelihood of tests not passing is going down every minute. I don't think I will be able to respond rapidly in the following 5 or so days though, so hopefully I will be able to deal with all problems today
Either is fine. If you expect to do this in the next couple of hours I can wait. Otherwise we can do it later and merge as "minor changes and improvements" :-)
OK, fair enough. Let me do it while I'm looking at it...
Mateusz Łoskot
@mloskot
We better give ourselves time for master CI
Olzhas Zhumabek
@simmplecoder
I'm a bit tired for today, did a lab and two reading reports. If the deadline is on Monday, I can incorporate the changes on weekends
Mateusz Łoskot
@mloskot
@simmplecoder Please, don't bother. You should go offline long time ago!
Olzhas Zhumabek
@simmplecoder
ok, thanks! good luck with the merge :)
Mateusz Łoskot
@mloskot
There will be no requests for you. If there is anything to tweak, I/we will do it
@stefanseefeld My comments were minor "nice to have" things. Since the Travis CI queue may take time, we better merge sooner and see if our master builds fine