Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(cli): [STENCIL-12] Adding Telemetry and CLI features. #2964
feat(cli): [STENCIL-12] Adding Telemetry and CLI features. #2964
Changes from 11 commits
f1deae7
628c0c7
d52a7d1
f524372
d889830
c635329
739e62c
cdf1879
dbbb0cb
aa5bb85
5ca6854
3d466ed
871ac40
60865ea
bce8319
6448a29
250f28c
66c8019
c251962
45f5a11
2e01743
b8986c7
f9644b8
063cc1c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please add tests on the return value now that we're returning a primitive? We should be able to assert that we return
true
whenwriteConfig
is successful, andfalse
when it failsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, great point! Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@splitinfinities did we get this under test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this adequate? https://github.com/ionic-team/stencil/pull/2964/files#diff-a2d702e7c67979d0440b8322026a5249d8206bbeaf3e73f7c84df87cfdd2d07cR69
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine ATM - normally I'd like us to get a little more granular there with respect to what we're testing (calling the
ionic-config
exposed functions directly) and splitting the tests out based on the assertion, but I think we can leave that for a future dayThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to my comment above, I'm not sure what the appropriate interface to use is here - should the
logger
be pulled fromgetStencilCLIConfig
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getLogger()
does pull from thegetStencilCLIConfig()
singleton - it's a helper method to return that similar togetCompilerSystem()
. I may also be misreading your question. Are you asking iflogger
should be stored on the singleton, as in philosophically?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me it seems like there's two ways to get a a logger instance:
As a user of the singleton, it's not exactly clear to me if there's a preferred way to get the logger without reading the code that the former is a wrapper around the latter. The ergonomics of pulling data off the singleton seems a little inconsistent to me - for some fields I need to call
getStencilCLIConfig()
, others have a helper function.That said, I'm not sure adding a helper function for each private method necessarily helps here. It gives us consistency/parity where there's a helper function for each private field that we expose via an accessor, but it doesn't answer the "which should I use?" question.
That's all a long winded way of saying "I think this is fine as is for now, because I don't have a great solution to offer up ATM". It's something we should be cognizant of as we evolve this class, but no action required ATM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally you'll see me throughout this PR use the helper functions, until I need to update the value, at which point I'll use:
getStencilCLIConfig().logger = updatedLogger
And the helper methods pretty much only try to help compensate line length.
Don't worry, I'm not married to this interaction, but at a minimum the singleton definitely helps.
We could get rid of the helper functions quickly, if you'd like me to do that let me know, I'm happy to!