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

Higher fidelity imports #21114

Closed

Conversation

techknowlogick
Copy link
Member

Currently if you want to track the history of various things when importing, they'd be added in as comments (ie, x added y label to issue).

This allows for additional fields in yaml, specifically issue comment type, as well as an interface rather than having to re-iterate all the ones that exist in the comment struct already.

@techknowlogick techknowlogick added the type/enhancement An improvement of existing functionality label Sep 8, 2022
@techknowlogick techknowlogick added this to the 1.18.0 milestone Sep 8, 2022
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 8, 2022
@techknowlogick techknowlogick changed the title WIP: Higher fidelity imports Higher fidelity imports Sep 27, 2022
@techknowlogick techknowlogick marked this pull request as ready for review September 27, 2022 00:31
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 27, 2022
PosterID int64 `yaml:"poster_id"`
PosterName string `yaml:"poster_name"`
PosterEmail string `yaml:"poster_email"`
Created time.Time
Updated time.Time
Content string
Reactions []*Reaction
Meta map[string]interface{} `yaml:"meta,omitempty"` // see models/issues/comment.go for fields in Comment struct
Copy link
Contributor

Choose a reason for hiding this comment

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

How does the value of Meta gets updated? I can only see some code reading it, but I can not find code writing it.

If let me decide, I would use clear field names instead of the dynamic map: the field names are clearly known, the dynamic map make the code like a dynamic language which loses the static compiling time check

Copy link
Member Author

Choose a reason for hiding this comment

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

It is written to via custom tooling to comments.yml and used during "restore-repo" CLI command.

I used an interface because this in theory could be extended to fit each field in the comments struct and I can import that struct due to cyclical imports.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is written to via custom tooling to comments.yml and used during "restore-repo" CLI command.

Is there a related PR to show how to use it?

I used an interface because this in theory could be extended to fit each field in the comments struct and I can import that struct due to cyclical imports.

I do not think the "in theory could be extended to fit each field" could really work with a dynamic map. In the end, on the reader side, every field is still pre-defined and clearly hard-coded in code. If it is the case, why not put some pointers like: AssigneeID *uint64, OldTitle *string.

@lunny lunny modified the milestones: 1.18.0, 1.19.0 Oct 23, 2022
@techknowlogick
Copy link
Member Author

closing in favour of #22510

@techknowlogick techknowlogick removed this from the 1.19.0 milestone Jan 18, 2023
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants