[GitHub] qpid-proton issue #160: WIP: option version of connection_options getters

2018-09-28 Thread codecov-io 
Github user codecov-io commented on the issue:

https://github.com/apache/qpid-proton/pull/160
  
# [Codecov](https://codecov.io/gh/apache/qpid-proton/pull/160?src=pr=h1) 
Report
> Merging 
[#160](https://codecov.io/gh/apache/qpid-proton/pull/160?src=pr=desc) into 
[master](https://codecov.io/gh/apache/qpid-proton/commit/ae96bced86c7e6c62655ee9b607ff57eabb056f1?src=pr=desc)
 will **decrease** coverage by `0.1%`.
> The diff coverage is `57.44%`.

[![Impacted file tree 
graph](https://codecov.io/gh/apache/qpid-proton/pull/160/graphs/tree.svg?width=650=UKKzV9XnFF=150=pr)](https://codecov.io/gh/apache/qpid-proton/pull/160?src=pr=tree)

```diff
@@Coverage Diff @@
##   master #160  +/-   ##
==
- Coverage   86.53%   86.43%   -0.11% 
==
  Files 252  253   +1 
  Lines   3077530817  +42 
==
+ Hits2663226636   +4 
- Misses   4143 4181  +38
```


| [Impacted 
Files](https://codecov.io/gh/apache/qpid-proton/pull/160?src=pr=tree) | 
Coverage Δ | |
|---|---|---|
| 
[cpp/include/proton/delivery\_mode.hpp](https://codecov.io/gh/apache/qpid-proton/pull/160/diff?src=pr=tree#diff-Y3BwL2luY2x1ZGUvcHJvdG9uL2RlbGl2ZXJ5X21vZGUuaHBw)
 | `50% <ø> (ø)` | :arrow_up: |
| 
[cpp/src/session\_options.cpp](https://codecov.io/gh/apache/qpid-proton/pull/160/diff?src=pr=tree#diff-Y3BwL3NyYy9zZXNzaW9uX29wdGlvbnMuY3Bw)
 | `0% <0%> (ø)` | :arrow_up: |
| 
[cpp/src/reconnect\_options.cpp](https://codecov.io/gh/apache/qpid-proton/pull/160/diff?src=pr=tree#diff-Y3BwL3NyYy9yZWNvbm5lY3Rfb3B0aW9ucy5jcHA=)
 | `47.05% <0%> (-19.61%)` | :arrow_down: |
| 
[cpp/src/session.cpp](https://codecov.io/gh/apache/qpid-proton/pull/160/diff?src=pr=tree#diff-Y3BwL3NyYy9zZXNzaW9uLmNwcA==)
 | `36.66% <100%> (+2.18%)` | :arrow_up: |
| 
[cpp/src/proactor\_container\_impl.cpp](https://codecov.io/gh/apache/qpid-proton/pull/160/diff?src=pr=tree#diff-Y3BwL3NyYy9wcm9hY3Rvcl9jb250YWluZXJfaW1wbC5jcHA=)
 | `86.12% <100%> (ø)` | :arrow_up: |
| 
[cpp/src/connection\_driver.cpp](https://codecov.io/gh/apache/qpid-proton/pull/160/diff?src=pr=tree#diff-Y3BwL3NyYy9jb25uZWN0aW9uX2RyaXZlci5jcHA=)
 | `84.84% <100%> (ø)` | :arrow_up: |
| 
[cpp/src/node\_options.cpp](https://codecov.io/gh/apache/qpid-proton/pull/160/diff?src=pr=tree#diff-Y3BwL3NyYy9ub2RlX29wdGlvbnMuY3Bw)
 | `61.11% <45.16%> (-9.02%)` | :arrow_down: |
| 
[cpp/src/connection\_options.cpp](https://codecov.io/gh/apache/qpid-proton/pull/160/diff?src=pr=tree#diff-Y3BwL3NyYy9jb25uZWN0aW9uX29wdGlvbnMuY3Bw)
 | `70.94% <54.71%> (-11.92%)` | :arrow_down: |
| 
[cpp/src/receiver\_options.cpp](https://codecov.io/gh/apache/qpid-proton/pull/160/diff?src=pr=tree#diff-Y3BwL3NyYy9yZWNlaXZlcl9vcHRpb25zLmNwcA==)
 | `70.68% <66.66%> (-10.02%)` | :arrow_down: |
| 
[cpp/src/sender\_options.cpp](https://codecov.io/gh/apache/qpid-proton/pull/160/diff?src=pr=tree#diff-Y3BwL3NyYy9zZW5kZXJfb3B0aW9ucy5jcHA=)
 | `64% <66.66%> (-2%)` | :arrow_down: |
| ... and [4 
more](https://codecov.io/gh/apache/qpid-proton/pull/160/diff?src=pr=tree-more)
 | |

--

[Continue to review full report at 
Codecov](https://codecov.io/gh/apache/qpid-proton/pull/160?src=pr=continue).
> **Legend** - [Click here to learn 
more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute  (impact)`, `ø = not affected`, `? = missing 
data`
> Powered by 
[Codecov](https://codecov.io/gh/apache/qpid-proton/pull/160?src=pr=footer). 
Last update 
[ae96bce...73af00f](https://codecov.io/gh/apache/qpid-proton/pull/160?src=pr=lastupdated).
 Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).



---

-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org

[GitHub] qpid-proton issue #160: WIP: option version of connection_options getters

2018-09-28 Thread alanconway 
Github user alanconway commented on the issue:

https://github.com/apache/qpid-proton/pull/160
  
Here's the final version, I'll hold off till after release to take a bit of 
time to consider API impact. @astitcher @ssorj 


---

-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org

[GitHub] qpid-proton issue #160: WIP: option version of connection_options getters

2018-09-28 Thread alanconway 
Github user alanconway commented on the issue:

https://github.com/apache/qpid-proton/pull/160
  
> Perhaps we could have a C++11 only explicit bool conversion as well as 
is_set(). However we would need to experiment a bit as some of the options 
themselves are also bools so this might be confusing!

I left it off because of the confusion. I could probably be persuaded but 
I'm not sure of the value - I think 
if (opt.is_set()) use(opt.get())
is clearer and not really harder to use than
if (opt) use(opt.get())

> Another thing we should consider is throwing an exception if get() is 
called when the option is not set.
No objection, I'll make it so. Technically it is a logic_error. Do you 
think  we should add proton::logic_error, or fudge it and just throw 
proton::error?



---

-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org

[GitHub] qpid-proton issue #160: WIP: option version of connection_options getters

2018-09-28 Thread astitcher 
Github user astitcher commented on the issue:

https://github.com/apache/qpid-proton/pull/160
  
I definitely like making option a class in common rather than 
duplicating it.

I think that this is the correct way to expose options in the API, but 
perhaps we could make the API a bit more easy to use. Perhaps we could have a 
C++11 only explicit bool conversion as well as is_set(). However we would need 
to experiment a  bit as some of the options themselves are also bools so this 
might be confusing!
Another thing we should consider is throwing an exception if get() is 
called when the option is not set.


---

-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org