Skip to content
This repository has been archived by the owner on Sep 21, 2023. It is now read-only.

Comment everything #11

Merged
merged 1 commit into from
Aug 7, 2017
Merged

Comment everything #11

merged 1 commit into from
Aug 7, 2017

Conversation

MorganEPatch
Copy link
Contributor

Depends on #10.

This PR introduces documentation comments to the entire command. This probably should have been done sooner.

@MorganEPatch MorganEPatch self-assigned this Jul 18, 2017
This was referenced Jul 18, 2017
Copy link
Contributor

@squat squat left a comment

Choose a reason for hiding this comment

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

Looks good. One minor comment: please make all comments full sentences with periods at the end. Once that is fixed, we can merge this.

cmd/root.go Outdated
ghStatusFieldID string // The customfield ID of the GitHub Status field in JIRA
ghReporterFieldID string // The customfield ID of the GitHub Reporter field in JIRA
isLastUpdateFieldID string // The customfield ID of the Last Issue-Sync Update field in JIRA
// since is the earliest GitHub issue updates we want to retrieve
Copy link
Contributor

Choose a reason for hiding this comment

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

these comments should all be full sentences with periods at the end.

@MorganEPatch
Copy link
Contributor Author

MorganEPatch commented Aug 7, 2017

@squat Is it better now?

cmd/root.go Outdated
@@ -863,6 +939,8 @@ func setLastUpdateTime() error {
return nil
}

// init sets the configuration variables of the command, then runs viper to
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a special function in Go. Do not document it

cmd/root.go Outdated
var jCommentIDRegex = regexp.MustCompile("^Comment \\(ID (\\d+)\\)")

// createCommments takes a GitHub issue, and retrieves all of its comments. It then
Copy link
Contributor

Choose a reason for hiding this comment

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

No comma needed here

cmd/root.go Outdated
@@ -559,6 +614,8 @@ func updateIssue(ghIssue github.Issue, jIssue jira.Issue, ghClient github.Client
return nil
}

// createIssue generates a JIRA issue from the various fields on the given GitHub issue, then
Copy link
Contributor

Choose a reason for hiding this comment

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

No comma needed

cmd/root.go Outdated
// compareIssues gets the list of GitHub issues updated since the `since` date,
// gets the list of JIRA issues which have GitHub ID custom fields in that list,
// then matches each one. If a JIRA issue already exists for a given GitHub issue,
// it calls updateIssue; if no JIRA issue already exists, it calls createIssue.
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment should not details the implementation and functions used but rather the business logic, e.g. "If the issue already exists, it is updated; if no issue exists, a new one is created." Otherwise, everytime implementation changes slightly, this needs to be kept in sync.

cmd/root.go Outdated
@@ -302,6 +346,8 @@ type JIRAField struct {
} `json:"schema,omitempty"`
}

// getFieldIDs requests the metadata of every issue field in the JIRA
// project, and saves the IDs of the custom fields used by issue-sync.
Copy link
Contributor

Choose a reason for hiding this comment

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

No comma needed

cmd/root.go Outdated
func GetJIRAClient(username, password, baseURL string) (*jira.Client, error) {
// getJIRAClient initializes a JIRA API client, then sets the Basic Auth credentials
// passed to it. (OAuth token support is planned.) It then requests the project using
// the key provided on the command line, to have it accessible by future functions and
Copy link
Contributor

Choose a reason for hiding this comment

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

no comma needed

cmd/root.go Outdated
func GetGitHubClient(token string) (*github.Client, error) {
// getGitHubClient initializes a GitHub API client with an OAuth client for authentication,
// then makes an API request to confirm that the service is running and the auth token
// is valid
Copy link
Contributor

Choose a reason for hiding this comment

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

Period

cmd/root.go Outdated
const commentDateFormat = "15:04 PM, January 2 2006"

// Execute provides a single function to run the root command and handle errors
Copy link
Contributor

Choose a reason for hiding this comment

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

Period

cmd/root.go Outdated
const dateFormat = "2006-01-02T15:04:05-0700"

// commentDateFormat is the format used in the headers of JIRA comments
Copy link
Contributor

Choose a reason for hiding this comment

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

Period

cmd/root.go Outdated
project jira.Project

// dryRun configures whether the application calls the create/update endpoints of the JIRA
// API, or just prints out the actions it would take.
Copy link
Contributor

Choose a reason for hiding this comment

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

No comma needed

This commit adds documentation comments on all of the methods
and variables in the file.
@MorganEPatch
Copy link
Contributor Author

@squat Those should be fixed.

Copy link
Contributor

@squat squat left a comment

Choose a reason for hiding this comment

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

:shipit:

@MorganEPatch MorganEPatch merged commit 2b16ece into coreos:master Aug 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants