-
Notifications
You must be signed in to change notification settings - Fork 85
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
[eas-cli] VCS Context Fields to replace helper functions #2086
Conversation
ENG-8958 Context field git command eas-cli
The task it to convert |
Size Change: +1.75 kB (0%) Total Size: 42.6 MB
|
Codecov Report
@@ Coverage Diff @@
## main #2086 +/- ##
==========================================
- Coverage 54.07% 54.04% -0.03%
==========================================
Files 508 509 +1
Lines 18674 18658 -16
Branches 3934 3933 -1
==========================================
- Hits 10097 10081 -16
Misses 7886 7886
Partials 691 691
|
/changelog-entry chore Move |
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.
This looks great and is a great step towards injecting more dependencies! See inline comment about a potential follow-up PR.
packages/eas-cli/src/commandUtils/context/VcsGitRepoExistsContextField.ts
Outdated
Show resolved
Hide resolved
|
||
export default class VcsClientContextField extends ContextField<Client> { | ||
async getValueAsync(): Promise<Client> { | ||
return getVcsClient(); |
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.
Ideally, in a follow-up PR, we could get rid of this function so that future command authors aren't tempted to just call this and instead are led to the "pit of success" by only being able to access this via a context. Of course, to do this will require more mocking in tests, so that's why I suggest doing this in a separate PR after this one.
For context (no pun intended), one of the original reasons for having context fields was to prevent command authors from calling getProjectAsync directly without calling a method to set it up. Now, all accesses to the project run the same code that ensures the project is set up correctly.
/changelog-entry chore Move |
✅ Thank you for adding the changelog entry! |
Why
Context fields make it easier for eas-cli commands to express their dependencies.
How
Created a new context field for
getVcsClient
andensureRepoExistsAsync
and replaced usages of their original helper function calls with references to EAS Command contexts.Test Plan
Run
yarn test
locally and all tests pass on CI.