Skip to content
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

Merged
merged 9 commits into from
Oct 18, 2023

Conversation

Josh-McFarlin
Copy link
Contributor

@Josh-McFarlin Josh-McFarlin commented Oct 13, 2023

Why

Context fields make it easier for eas-cli commands to express their dependencies.

How

Created a new context field for getVcsClient and ensureRepoExistsAsync 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.

image

@linear
Copy link

linear bot commented Oct 13, 2023

ENG-8958 Context field git command eas-cli

The task it to convert getVcsClient and ensureRepoExistsAsync into a command context. Talk to wschurman about how to do this.

@github-actions
Copy link

github-actions bot commented Oct 13, 2023

Size Change: +1.75 kB (0%)

Total Size: 42.6 MB

Filename Size Change
./packages/eas-cli/dist/eas-linux-x64.tar.gz 42.6 MB +1.75 kB (0%)

compressed-size-action

@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Merging #2086 (f751c75) into main (5ed155e) will decrease coverage by 0.03%.
The diff coverage is 33.83%.

@@            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              
Files Coverage Δ
packages/eas-cli/src/build/ios/build.ts 35.56% <ø> (ø)
packages/eas-cli/src/commandUtils/EasCommand.ts 96.78% <100.00%> (+0.06%) ⬆️
.../src/commandUtils/context/VcsClientContextField.ts 100.00% <100.00%> (ø)
packages/eas-cli/src/commands/branch/create.ts 45.17% <ø> (ø)
packages/eas-cli/src/commands/build/cancel.ts 51.32% <ø> (ø)
packages/eas-cli/src/commands/build/index.ts 20.54% <ø> (ø)
packages/eas-cli/src/commands/build/internal.ts 63.16% <ø> (ø)
packages/eas-cli/src/commands/build/list.ts 33.34% <ø> (ø)
packages/eas-cli/src/commands/build/resign.ts 30.00% <ø> (ø)
packages/eas-cli/src/commands/build/run.ts 21.22% <ø> (ø)
... and 53 more

@Josh-McFarlin
Copy link
Contributor Author

/changelog-entry chore Move getVcsClient and ensureRepoExistsAsync into command context. (#2086 by @Josh-McFarlin)

@Josh-McFarlin Josh-McFarlin marked this pull request as ready for review October 17, 2023 18:11
@Josh-McFarlin Josh-McFarlin changed the title VCS Context Fields to replace helper functions [eas-cli] VCS Context Fields to replace helper functions Oct 17, 2023
Copy link
Member

@wschurman wschurman left a 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.


export default class VcsClientContextField extends ContextField<Client> {
async getValueAsync(): Promise<Client> {
return getVcsClient();
Copy link
Member

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.

@Josh-McFarlin
Copy link
Contributor Author

/changelog-entry chore Move getVcsClient and ensureRepoExistsAsync into command context. (#2086 by @Josh-McFarlin)

@github-actions
Copy link

✅ Thank you for adding the changelog entry!

@Josh-McFarlin Josh-McFarlin merged commit 743ae7c into main Oct 18, 2023
9 checks passed
@Josh-McFarlin Josh-McFarlin deleted the eng-8958-git-context-field branch October 18, 2023 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants