-
Notifications
You must be signed in to change notification settings - Fork 30
feat: add tibia_urls field into information-section #398
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: add tibia_urls field into information-section #398
Conversation
There was a problem hiding this 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 @Kai-Animator, this is a good start!
There are some things that I'd like to be adjusted.
One of them is that there is already a link for every endpoints source available, so hardcoding the line in the "Link" field is not optimal. It would be better to have it being populated by the value set in "tibiadataRequest.URL" before being sent to tibiaDataRequestHandler.
For instance here:
tibiadata-api-go/src/webserver.go
Lines 394 to 407 in a4afbe1
func tibiaFansites(c *gin.Context) { | |
tibiadataRequest := TibiaDataRequestStruct{ | |
Method: resty.MethodGet, | |
URL: "https://www.tibia.com/community/?subtopic=fansites", | |
} | |
tibiaDataRequestHandler( | |
c, | |
tibiadataRequest, | |
func(BoxContentHTML string) (interface{}, error) { | |
return TibiaFansitesImpl(BoxContentHTML) | |
}, | |
"TibiaFansites") | |
} |
I'd also like the "Link" to be named "TibiaURL" instead, since that's a slightly better description of that it is.
Also, we should probably want to have a []string as well, in case we add multi-upstream sources.
Thanks for the review @tobiasehlert . I will work on it 👍 |
Hi @tobiasehlert 👋 I’ve implemented the changes you requested:
Please let me know if there’s anything else :] |
Since this is now slice, I think it should probably be named |
Yeah good input @phenpessoa. 👍 |
The field name was updated but the json is still called |
Hey @phenpessoa, thanks for looking at the code, is this the change that you mentioned? |
Yup! Looks good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are getting closer! 🚀
The news endpoints just need some adjustment, but then we should be really to rock.
Thanks for the feedback @tobiasehlert, sorry search and replace all because I though you weren't using TibiaURL in other files. 🙇 Hope this revisions are satisfying. |
…s / Add test for information on characters test
…and guilds/:world endpoints
2545427
to
758f09c
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me 🌟
This pull request addresses Issue #381 by adding the
Link
field to theInformation
section in all API responses. TheLink
field provides a direct URL to the source data on the official Tibia website.Changes Overview:
Information
Struct:Link
field to theInformation
struct inwebserver.go
:Updated all API endpoint handlers to populate the
Link
field with the appropriate URL pointing to the source data.Example from
TibiaWorldsWorld.go
:Testing:
Link
field is correctly populated.Notes:
NewsList was kind of tricky to handle the different patterns so I decided to sum all different URLs into only "https://www.tibia.com/news/?subtopic=newsarchive"
This is my first PR in the project so I am open to feedback and willing to make any necessary changes to meet the project's standards.
fix #381