Revisit pen api #257
Revisit pen api #257
Conversation
|
I want to have a good comparison with the changes to pens in #211 There's likely some overlap. |
| /// <summary> | ||
| /// Options for the Pen | ||
| /// </summary> | ||
| public class PenOptions |
There was a problem hiding this comment.
I wonder if we'd be better with get; init; patter here. I'd make JointStyle and EndCapStyle non-nullable also providing defaults.
There was a problem hiding this comment.
Yea. But I would really go for the required keyword. But for that we need to use c#11 and I du not know if this is bound to net7
There was a problem hiding this comment.
That's off the table unfortunately. Some stuff works, some stuff doesn't but looks like it would. I'm not sure we can take the risk.
There was a problem hiding this comment.
I wonder if we'd be better with
get; init;patter here.
init-only setters officially require .NET 5+, so we would need to drop the old targets, which should be a bigger strategic decision. I recommend to avoid doing it in this PR, and revisit the problem when we switch to .NET 6.0+.
There was a problem hiding this comment.
Ah ok. We can do that then. Thanks!
| public Pen(Color color, float width) | ||
| : this(new SolidBrush(color), width) | ||
| public Pen(PenOptions options) | ||
| : this(options.StrokeFill, options.StrokeWidth) |
There was a problem hiding this comment.
PenOptions contains the joint/endcap properties but we're not using them
Codecov Report
@@ Coverage Diff @@
## main #257 +/- ##
===================================
Coverage 71% 71%
===================================
Files 88 89 +1
Lines 5372 5395 +23
Branches 1098 1098
===================================
+ Hits 3853 3872 +19
- Misses 1300 1304 +4
Partials 219 219
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@stefannikolei any reason you made |
Because of #195 (comment) |
|
There's a chance those options might be removed as part of #211 I'm just merging the conflicts there now. If not, i'll revisit for UX |
That comment actually proposes |
🙈 yeah you are absolute right. |
|
I've got it covered |
Prerequisites
Description
First spike for Revisit Pen API #195. As a discussion base.
Is this how it was described in the issue or should it be totally different?