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.

Sign up for GitHub

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

Jump to bottom

feat(packages): ensure git dependencies are reachable #379

Merged
merged 2 commits into from
May 6, 2022
Merged

feat(packages): ensure git dependencies are reachable #379

merged 2 commits into from
May 6, 2022

Conversation

felangel
Copy link
Contributor

@felangel felangel commented May 6, 2022

Description

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

@felangel felangel added the feature A new feature or request label May 6, 2022
@felangel felangel self-assigned this May 6, 2022
@felangel felangel marked this pull request as ready for review May 6, 2022 18:39
@felangel felangel requested a review from jorgecoca as a code owner May 6, 2022 18:39
wolfenrain
wolfenrain approved these changes May 6, 2022
View reviewed changes
Copy link
Member

@wolfenrain wolfenrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small nit, but lgtm

test/src/cli/git_cli_test.dart
group('reachable', () {
test('completes for a reachable remote', () async {
await expectLater(
Git.reachable(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test would fail if you don't have GIT on your system or in the CI right? While that would be strange to not have, it would fail incorrectly cause the test itself would probably be fine.

Should we maybe mock the _Cmd somehow?

Copy link
Contributor Author

@felangel felangel May 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was planning to create a separate issue to refactor all CLI classes to support an injectable Process rather than exposing static methods. Thoughts on making this update in a subsequent PR? If git is not installed on the system then pub get/flutter packages get would also fail.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True but technically that could be skipped by the user, but I am fine with improving it for the tests on a separate PR!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#380

@felangel felangel merged commit 886bb85 into main May 6, 2022
@felangel felangel deleted the feat/git-dependencies-reachability branch May 6, 2022 19:25
@felangel felangel mentioned this pull request May 6, 2022
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wolfenrain wolfenrain wolfenrain approved these changes

@renancaraujo renancaraujo Awaiting requested review from renancaraujo

@orestesgaolin orestesgaolin Awaiting requested review from orestesgaolin

@jorgecoca jorgecoca Awaiting requested review from jorgecoca

Assignees

@felangel felangel

Labels
feature A new feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fix: very_good packages get gets stuck when using dependency via git https link but not authenticated
2 participants
@felangel @wolfenrain