Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
  • May 08 20:59
    mloskot milestoned #606
  • May 08 20:59
    mloskot labeled #606
  • May 08 20:59
    mloskot labeled #606
  • May 08 20:59
    mloskot opened #606
  • May 08 20:57
    mloskot commented #453
  • May 08 08:25
    Build #185 passed
  • May 08 07:33
    Sayan-Chaudhuri review_requested #604
  • May 08 07:31
    Sayan-Chaudhuri synchronize #604
  • May 08 07:31
    Sayan-Chaudhuri synchronize #604
  • May 08 07:28
    Sayan-Chaudhuri synchronize #604
  • May 07 08:11
    mloskot commented #599
  • May 07 08:09
    mloskot commented #599
  • May 07 05:54
    sdebionne commented #453
  • May 07 02:08
    lpranam commented #599
  • May 06 23:43
    mloskot edited #599
  • May 06 23:41
    mloskot commented #596
  • May 06 23:40
    mloskot commented #596
  • May 06 23:39
    mloskot commented #596
  • May 06 23:34
    mloskot synchronize #596
  • May 06 23:23
    mloskot commented #596
Mateusz Łoskot
@mloskot
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
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
Mateusz Łoskot
@mloskot
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)
Mateusz Łoskot
@mloskot
@lpranam Can I merge #398?
Pranam Lashkari
@lpranam
@mloskot sure
Mateusz Łoskot
@mloskot
@lpranam Done, thanks
Olzhas Zhumabek
@simmplecoder
@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
Mateusz Łoskot
@mloskot
No problem. You have different priorities now
Mateusz Łoskot
@mloskot
Olzhas Zhumabek
@simmplecoder
@mloskot, one of the builds broke due to some changes in the OS or something similar, I think
Mateusz Łoskot
@mloskot
Restarting the job. If does not help, we will ignore it.
Olzhas Zhumabek
@simmplecoder
great, seems like it worked. Thanks!
Mateusz Łoskot
@mloskot
Awesome!
I will keep nudging @stefanseefeld :-)
Olzhas Zhumabek
@simmplecoder
merged, thanks @stefanseefeld !
Mateusz Łoskot
@mloskot
@simmplecoder Do you mind if I run apply some of the corrections/changes I suggested in my review?
Stefan Seefeld
@stefanseefeld
OK, so I could merge to master now ?
Olzhas Zhumabek
@simmplecoder
@mloskot , sure, if we've got time
Stefan Seefeld
@stefanseefeld
Oh, OK, I'll wait a little longer, then.
:-)
Mateusz Łoskot
@mloskot
It's not critical for release, I can do it after merge, for example https://github.com/boostorg/gil/pull/392#discussion_r338236303
Stefan Seefeld
@stefanseefeld
I'll merge no later than tonight (EST)
Mateusz Łoskot
@mloskot
I'm also a bit :confused: about the length namin https://github.com/boostorg/gil/pull/392#discussion_r338234886
Stefan Seefeld
@stefanseefeld
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" :-)
Mateusz Łoskot
@mloskot
Finally, it would be nice to stick in some asserts https://github.com/boostorg/gil/pull/392#discussion_r338237322
@stefanseefeld Let's merge it now
Stefan Seefeld
@stefanseefeld
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
Stefan Seefeld
@stefanseefeld
Er, now I'm confused: I just updated both master and develop, then I tried to merge develop into master, but I get merge conflicts. How is that possible ?
Mateusz Łoskot
@mloskot
Where is the conflict?
I'm trying myself
Stefan Seefeld
@stefanseefeld
a whole lot ! mostly build logic, i.e. Jamfiles, CMakeLists.txt files, but also some json.
Mateusz Łoskot
@mloskot
Yes, I see them. This is weird as I did try the merge some weeks testing the emergency fix for our docs and it was fine
Stefan Seefeld
@stefanseefeld
right, so you cherry-picked. I didn't expect this to cause merge-conflicts down the line, though. Hmm.
Mateusz Łoskot
@mloskot
me neither
I'm looking what's wrong
Stefan Seefeld
@stefanseefeld
thanks
Mateusz Łoskot
@mloskot
@stefanseefeld you there?
Stefan Seefeld
@stefanseefeld
yes