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

Improve the SubmitRecord structure #168

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Ai-feier
Copy link

The SubmitRecords field was added to ChangeInfo. The following is an example I obtained through rest request, gerrit:3.10:
image
image
image
my request:
image

@Ai-feier
Copy link
Author

Just add the SubmitRecords field in ChangeInfo, I hope to join as soon as possible

Copy link
Owner

@andygrunwald andygrunwald left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.
I left a few comments on how to adjust the pull request more towards the original Gerrit documentation.

changes.go Outdated
Status string `json:"status"`
AppliedBy *Account `json:"applied_by,omitempty"`
}

// SubmitRecord entity describes results from a submit_rule.
type SubmitRecord struct {
Copy link
Owner

Choose a reason for hiding this comment

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

According to the docs, SubmitRecordInfo (https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#submit-record-info) and SubmitRecord (https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#submit-record) are different types. I would prefer if we create two different types as well.

Copy link
Author

Choose a reason for hiding this comment

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

In a new commit I have modified

changes.go Outdated
// SubmitRecord entity describes results from a submit_rule.
type SubmitRecord struct {
RuleName string `json:"rule_name"`
Copy link
Owner

Choose a reason for hiding this comment

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

Please extend the ,omitempty tags

Copy link
Author

Choose a reason for hiding this comment

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

In a new commit I have modified

changes.go Outdated
Comment on lines 226 to 232
type Account struct {
AccountId int `json:"_account_id"`
Name string `json:"name"`
Email string `json:"email"`
Username string `json:"username"`
Tags []string `json:"tags,omitempty"`
}
Copy link
Owner

Choose a reason for hiding this comment

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

This is not needed, as we need an AccountInfo type

Copy link
Author

Choose a reason for hiding this comment

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

In a new commit I have modified

@Ai-feier
Copy link
Author

I made modifications according to relevant requirements, and the current project does not have good support for approvals in the event structure. The main added oldvalue field
image

@Ai-feier
Copy link
Author

I want to know why it is designed like this
image

@Ai-feier
Copy link
Author

please review the code

@andygrunwald
Copy link
Owner

I made modifications according to relevant requirements, and the current project does not have good support for approvals in the event structure. The main added oldvalue field image

@Ai-feier Changing the type here would be a breaking change. I do agree, it is not a great Dev Experience on this one, however, it would be a breaking change. On top of this, i suggest we split the use cases and topics here.

I suggest:

  1. Having a new issue to discuss the design of attributes from SubmitRecordIngo
  2. Having a new Pull Request to change the type from value and oldValue from string to int
  3. Having a Pull request with extending the missing fields zu SubmitRecord

This way, we can

  • decide on what we merge
  • keep the changes isolated
  • don't mix topics and discussions are easier to follow

What do you think?

@Ai-feier
Copy link
Author

You also use int for value and oldValue. Many times odlValue will not be able to obtain the value because it contains special characters such as: ", /. It is true that this modification is destructive, but it is necessary for the subsequent complex development of gerrit. Yes, I have used the latest version in our company.

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.

2 participants