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

customize json tag #15

Closed
wants to merge 6 commits into from
Closed

customize json tag #15

wants to merge 6 commits into from

Conversation

liov
Copy link

@liov liov commented Sep 11, 2020

No description provided.

Copy link
Contributor

@ydnar ydnar 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 pull request. We’re probably not going to merge it, but I appreciate the PR.

Since there wasn’t a description in the PR, I’m trying to guess at what the purpose is. Why have a separate json_tags option when tags can be used to specify the json tags for a struct field?

Second, the PR renames the package and module from alta to liov. All of these would need to be backed out and rebased/squashed before we consider merging this.

Note: you don’t need to rename a a Go module or package to work on a fork of it. You can just use a replace directive in the go.mod file to point a module at a local directory or fork:

https://thewebivore.com/using-replace-in-go-mod-to-point-to-your-local-module/

README.md Outdated Show resolved Hide resolved
patch/patcher.go Outdated Show resolved Hide resolved
patch/patcher.go Outdated Show resolved Hide resolved
@liov
Copy link
Author

liov commented Sep 13, 2020

Sorry, I found that the tags option cannot generate custom json tags, so I wrote a json_tag tag myself, and later changed the package name because I think this PR will not be merged for the time being and I want to use it for my own project

@ydnar
Copy link
Contributor

ydnar commented Sep 13, 2020

I see. I'd accept a pull request that allows the tags directive to override the json tag.

@liov liov requested a review from ydnar September 14, 2020 06:05
@liov liov force-pushed the main branch 4 times, most recently from e0fe946 to b54f54c Compare September 14, 2020 11:22
@liov
Copy link
Author

liov commented Sep 14, 2020

I think I understand what you mean

@liov liov marked this pull request as draft September 14, 2020 11:36
@liov
Copy link
Author

liov commented Sep 14, 2020

now,the tags directive to override the json tag.But I reset the branch and forced push it, I don’t know if this is a valid PR

patch/patcher.go Outdated Show resolved Hide resolved
patch/patcher.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ydnar ydnar left a comment

Choose a reason for hiding this comment

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

OK. Now it needs tests!

@liov
Copy link
Author

liov commented Sep 16, 2020

I think the tags order is not very important

@liov liov requested a review from ydnar September 16, 2020 01:53
Copy link
Contributor

@ydnar ydnar left a comment

Choose a reason for hiding this comment

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

Thanks!

I’d love to see the new logic extract into a simple helper function that can have its own unit tests, ala:

func mergeTags(tags, newTags string) string {
        ...
}

patch/patcher.go Outdated Show resolved Hide resolved
@liov liov requested a review from ydnar September 18, 2020 01:55
@liov liov force-pushed the main branch 3 times, most recently from 6a1ebff to 1317867 Compare September 18, 2020 03:39
@liov liov marked this pull request as ready for review September 27, 2020 06:35
patch/patcher.go Outdated Show resolved Hide resolved
patch/patcher.go Outdated Show resolved Hide resolved
patch/patcher_test.go Outdated Show resolved Hide resolved
patch/patcher.go Outdated Show resolved Hide resolved
@liov liov requested a review from ydnar October 10, 2020 08:36
patch/patcher_test.go Outdated Show resolved Hide resolved
patch/patcher_test.go Outdated Show resolved Hide resolved
patch/patcher.go Outdated Show resolved Hide resolved
@liov liov requested a review from ydnar October 13, 2020 07:43
Copy link
Contributor

@ydnar ydnar left a comment

Choose a reason for hiding this comment

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

The test rewrites look terrific, thank you!

patch/patcher.go Outdated Show resolved Hide resolved
@liov liov requested a review from ydnar October 14, 2020 08:15
@ydnar
Copy link
Contributor

ydnar commented Nov 24, 2020

Hi @liov! Thanks for the updates in the last commit.

Would it make sense to import and use this package instead of reimplementing it here? https://pkg.go.dev/github.com/fatih/structtag

@ydnar
Copy link
Contributor

ydnar commented Dec 6, 2020

@ydnar
Copy link
Contributor

ydnar commented Dec 24, 2020

Superseded by #31. Thanks @liov!

@ydnar ydnar closed this Dec 24, 2020
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