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

Add git info to check/push #205

Merged
merged 17 commits into from
Feb 1, 2021
Merged

Add git info to check/push #205

merged 17 commits into from
Feb 1, 2021

Conversation

JakeDawkins
Copy link
Contributor

@JakeDawkins JakeDawkins commented Jan 27, 2021

Adds the ability to pass git info to all check/push commands for use in Apollo Studio

@JakeDawkins JakeDawkins added feature 🎉 new commands, flags, functionality, and improved error messages status - needs review labels Jan 29, 2021
@JakeDawkins JakeDawkins mentioned this pull request Jan 29, 2021
src/cli.rs Outdated

// constructing GitContext with a set of overrides from env vars
let git_context = GitContext::new(branch, committer, commit, remote_url);
tracing::debug!("Git Context: {:?}", git_context);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tracing::debug!("Git Context: {:?}", git_context);
tracing::debug!("Git Context", ?git_context);

Copy link
Contributor Author

@JakeDawkins JakeDawkins Jan 29, 2021

Choose a reason for hiding this comment

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

This doesn't work for me, I get an error saying

error: expected expression, found `?`
  --> src/cli.rs:97:40
   |
97 |         tracing::debug!("Git Context", ?git_context);
   |                                        ^ expected expression

src/git.rs Outdated
) -> Self {
let git = git_info::get();

let branch = if override_branch.is_none() {
Copy link
Contributor

Choose a reason for hiding this comment

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

these could all be one liners

let branch = override_branch.unwrap_or_else(|| git.current_branch);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't quite work, because git.current_branch is an Option<String>, so the return types clash of it either being a string from the unwrapping of override_branch or an Option<String> from git.

Copy link
Member

Choose a reason for hiding this comment

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

you're looking for or()! So:

let branch = override_branch.or(git.current_branch);

src/git.rs Outdated
if user.is_empty() && email.is_empty() {
None
} else {
Some(format!("{} {}", user, email))
Copy link
Contributor

Choose a reason for hiding this comment

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

this gets a bit murky if user is empty and email is not - wonder if that's a common case

Copy link
Member

Choose a reason for hiding this comment

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

definitely a common case i've seen a whole bunch

src/git.rs Outdated
Comment on lines 121 to 125
let remote_url = if let Some(remote) = remote_url {
GitContext::sanitize_remote_url(&remote)
} else {
None
};
Copy link
Contributor

Choose a reason for hiding this comment

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

let remote_url = remote_url.map(|url| GitContext::sanitize_remote_url(&url));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The result of this would also be the wrong type, it'd be Option<Option<String>> instead of Option<String>

JakeDawkins and others added 8 commits January 31, 2021 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🎉 new commands, flags, functionality, and improved error messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants