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

[feat]: [PL-24913]: Add the support for create and update file in bitbucket server #177

Merged
merged 8 commits into from
May 9, 2022

Conversation

DeepakPatankar
Copy link
Contributor

No description provided.

@tphoney tphoney merged commit 78b5e76 into drone:master May 9, 2022
Copy link

@allofthesepeople allofthesepeople left a comment

Choose a reason for hiding this comment

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

mostly comments and suggestions.

w := multipart.NewWriter(&b)
// add the other fields
if content.Message != "" {
_ = w.WriteField("content", string(content.Content))

Choose a reason for hiding this comment

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

please handle these errors. Aware they're repetitive but could be more palatable with something like:

type multipartWriter struct {
    w mime. Writer
    err error
}

func (multipartWriter *mw) Write(f, v string) {
    if mw.err == nil {
        return
    }
    if v == "" {
        return
    }
    _, err = mw.WriteField(f, v)    
}
case *contentCreateUpdate:
    var b bytes.Buffer
    mw := multipartWriter{w: multipart.NewWriter(&b)}
    mw.write("content", string(content.Content))
    mw.write("message", content.Message)
    mw.write("branch", content.Branch)
    mw.write("sha", content.Sha)
    if mw.err != nil {
        return nil, fmt.Errorf("error writing multipart-content. err: %s", mw.err)
    }

see https://go.dev/blog/errors-are-values for background

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did this change in another PR

var b bytes.Buffer
w := multipart.NewWriter(&b)
// add the other fields
if content.Message != "" {

Choose a reason for hiding this comment

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

is there a reason to check for empty value? Or in the name of readability might it be easier to pass on the empty fields? (there might be, I'm unfamiliar with the API)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some of the cases this values are not event required for the bitbucket API, hence I added this check that if the value is not null then don't add this value to multipart data

@@ -53,6 +68,13 @@ type contents struct {
Values []string `json:"values"`
}

type contentCreateUpdate struct {

Choose a reason for hiding this comment

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

would a name like "multipartContent" or "multipartUploadContent" be better here?

Also suggest removing the json tags as it's a little confusing to the reader; the purpose of this struct appears to be in the case where json is not used.

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 current design in scm service is that whenever we are calling the client.do method, we pass it the json value and the client.do method converts it into the multipart data.
Because of which we cannot name it to multipartContent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants