Convert XConfigure into constructors #1155
Conversation
Previously, we discussed the possibility of converting the config types into internal ones. But due to the cyclic dependencies it introduces, we are only converting XConfigure into constructors and document that XConfig types are most likely are not going to be directly used by developers. In package documents, constructors will be nicely listed under the config types and they won't be yet another standalone symbol developers need to learn about. Fixes #1130.
rakyll
requested review from
Aneurysm9,
evantorrie,
jmacd,
lizthegrey,
MrAlias and
XSAM
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1155 +/- ##
========================================
- Coverage 78.0% 77.7% -0.4%
========================================
Files 135 135
Lines 7124 7153 +29
========================================
Hits 5559 5559
- Misses 1321 1350 +29
Partials 244 244
|
|
Please provide a |
|
Added changes to the CHANGELOG, PTAL. |
There was a problem hiding this comment.
This naming seems to be in conflict with the existing style guide. However, based on what you pointed out about the grouping of this as a constructor for the related struct it seems like this is a better design. I think we need a follow-on issue to update to this if we all agree this is the prefered approach.
|
It seems like the existing
If we decide to move to this naming pattern it seems like it would be more motivated to align with the canonical Go approach. |
|
@MrAlias, you are right. It's already aligning it -- the PR description need to be edited. I think due to the name, it looks like it's something else. If we want to go with this merge, it may improve that perception. |
|
Updated the description. |
|
I think this is the correct direction. Opened #1160 to address the change. |
* Convert XConfigure into constructor for metrics A follow up of #1155. * Add to CHANGELOG Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Previously, we discussed the possibility of converting
the config types into internal ones. But due to the
cyclic dependencies it introduces, we are only
converting XConfigure into constructors and document that
XConfig types are most likely are not going to be directly
used by developers.
By naming, it makes it clear the constructors are tightly
related to the config types and not yet another way
to configure the package. It should improve the mental overhead.
Fixes #1130.