refactor(cli): inject CLI version as a parameter to Notices #323
Conversation
| } | ||
| } | ||
|
|
||
| ioHelper = ioHelper ?? CliIoHost.instance().asIoHelper(); |
There was a problem hiding this comment.
this was left over. ioHelper is a required argument, so the second part never happens anyway
There was a problem hiding this comment.
these functions look like they're copied from aws-cdk/cli/version.ts. do you mean to copy them over without deleting their duplicates in aws-cdk?
There was a problem hiding this comment.
yes, they will be removed eventually
why?
why? |
kaizencc
left a comment
kaizencc
left a comment
There was a problem hiding this comment.
approved but would like the PR description to be updated to explain why these changes are necessary
Notices will serve both toolkit-lib and cli. having a hard coded "cli version" that is actually reading the version from the current package will be wrong. In future this will be extended, for now I'm just removing the hard dependency. |
mrgrain
force-pushed
the
mrgrain/notices/fix
branch
from
8eff2c5 to
5277d70
Compare
`Notices` will eventually serve both toolkit-lib and cli. Currently the class has a hard-coded "cli version" - but really the value is reading the version from whatever the current package is. Once we start using this file in another package, it will be wrong. For now, let's just remove the hard dependency on cli version inside the class. In future it will be extended. - Remove direct dependency on versionNumber import in Notices class - Add cliVersion as a required parameter in NoticesProps interface - Store cliVersion as a class property and use it in display method - Improve testability by removing the need to mock version functions
mrgrain
force-pushed
the
mrgrain/notices/fix
branch
from
5277d70 to
93728ce
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #323 +/- ##
==========================================
+ Coverage 86.72% 86.91% +0.18%
==========================================
Files 218 219 +1
Lines 35583 35609 +26
Branches 4347 4359 +12
==========================================
+ Hits 30860 30948 +88
+ Misses 4623 4566 -57
+ Partials 100 95 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
`Notices` will eventually serve both toolkit-lib and cli. Currently the class has a hard-coded "cli version" - but really the value is reading the version from whatever the current package is. Once we start using this file in another package, it will be wrong. For now, let's just remove the hard dependency on cli version inside the class. In future it will be extended. - Remove direct dependency on versionNumber import in Notices class - Add cliVersion as a required parameter in NoticesProps interface - Store cliVersion as a class property and use it in display method - Improve testability by removing the need to mock version functions --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
3 participants
Noticeswill eventually serve both toolkit-lib and cli. Currently the class has a hard-coded "cli version" - but really the value is reading the version from whatever the current package is. Once we start using this file in another package, it will be wrong. For now, let's just remove the hard dependency on cli version inside the class. In future it will be extended.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license