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

Add language statistics API endpoint #11737

Merged
merged 8 commits into from
Jun 7, 2020

Conversation

lafriks
Copy link
Member

@lafriks lafriks commented Jun 2, 2020

Fixes #10232

@lafriks lafriks added type/feature Completely new functionality. Can only be merged if feature freeze is not active. modifies/api This PR adds API routes or modifies them labels Jun 2, 2020
@lafriks lafriks added this to the 1.13.0 milestone Jun 2, 2020
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 2, 2020
@CirnoT
Copy link
Contributor

CirnoT commented Jun 3, 2020

Why use custom and not json?

@6543
Copy link
Member

6543 commented Jun 3, 2020

I dont know how hard this is - but tests would be nice

@lafriks
Copy link
Member Author

lafriks commented Jun 4, 2020

@CirnoT to return languages in descending order by respective code size in bytes. Map serialization would return them in alphabetical order

@CirnoT
Copy link
Contributor

CirnoT commented Jun 4, 2020

JSON map objects have undefined order, no one should depend on what order we return them in - if they do, their code is bound to break on something at some point, 100% guaranteed.

@lafriks
Copy link
Member Author

lafriks commented Jun 4, 2020

that's why I don't use map and use custom json serialization

@CirnoT
Copy link
Contributor

CirnoT commented Jun 4, 2020

You don't understand me, it's not Go spec, its JSON spec. You shall not return ordered or depend on order of JSON objects, EVER.

In fact you can not even rely on what order you will get after you use some language built-in function to fetch that JSON string from our API endpoint.

@CirnoT
Copy link
Contributor

CirnoT commented Jun 4, 2020

In other words, If you have code like this:

JSON.parse('{"PHP":23334, "Go":6754}')

And rely or expect that PHP will always be first on iteration, you are doing things wrong.

@CirnoT
Copy link
Contributor

CirnoT commented Jun 4, 2020

The reason why GitHub returns them ordered is most likely because they internally iterate in that way, not because they TRY to present such order.

@lafriks
Copy link
Member Author

lafriks commented Jun 4, 2020

But if you use JSON reader like parsing than you will get primary language first. imho github specifically returns them it this order not just by coincidence

@lafriks
Copy link
Member Author

lafriks commented Jun 4, 2020

Test added

@CirnoT
Copy link
Contributor

CirnoT commented Jun 4, 2020

I can not agree with explicitly suggesting order and encouraging developers to rely on it; it's just wrong.

@CirnoT
Copy link
Contributor

CirnoT commented Jun 4, 2020

But if you use JSON reader like parsing than you will get primary language first

Today it works for you, tomorrow you will update unrelated dependency and it won't work anymore because you were relying on something that is strictly against JSON spec.

@lafriks
Copy link
Member Author

lafriks commented Jun 4, 2020

It's not against spec, against spec would be if json was invalid. There is no reason why this code would start returning them in different order. There are languages that has ordered map classes that would decode this structure just fine, if you just use default JSON.parse it will work same no matter in what order we return them

@CirnoT
Copy link
Contributor

CirnoT commented Jun 4, 2020

https://www.json.org/json-en.html

An object is an unordered set of name/value pairs

As a consequence, JSON libraries are free to rearrange the order of the elements as they see fit

@CirnoT
Copy link
Contributor

CirnoT commented Jun 4, 2020

Your code attempts to explicitly return object in specific order, and by your explanation I fully expect that you believe you can write code that will assume first element to be always primary language. That is incorrect. Some libraries will preserve input order, some will re-arrange it alphabetically, others will try to move integer values above other values.

I don't mind any specific order, what I have issue is that you are forcing specific order and to do that, you go to insane lengths of writing own MarshalJSON.

@CirnoT
Copy link
Contributor

CirnoT commented Jun 4, 2020

For example, if someone were to naively use our API in Go and expect first element to by primary language, he would be very surprised by the result: https://play.golang.org/p/XYUrtJlEG_b

As such, I don't see any point on keeping any order for what we return, especially when it requires additional code that is subject to breakage.

@lafriks
Copy link
Member Author

lafriks commented Jun 4, 2020

I understand all that but that does not mean we can not return it using same order as github to be as compatible with them as possible. It's up to the API consumer to either to depend on it or not

@lafriks
Copy link
Member Author

lafriks commented Jun 4, 2020

As for MarshalJSON length it's not that insanely big or complex

@CirnoT
Copy link
Contributor

CirnoT commented Jun 4, 2020

to be as compatible with them as possible

It's compatible either way

It's up to the API consumer to either to depend on it or not

No, it's not. Attempting to depend on it is incorrect.

@lafriks
Copy link
Member Author

lafriks commented Jun 4, 2020

I do understand your point but I don't see much of the problem returning exactly like github does and I can bet they do that order intentionally.
Also not that it changes much but this way it is less memory consuming :]

Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

beside the none default way how it is converted ... I'm ok with this

works smothly 🙃

@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 Jun 4, 2020
@CirnoT
Copy link
Contributor

CirnoT commented Jun 4, 2020

I am not convinced that your arguments warrant introducing custom JSON marshal function; I'll let someone else review it.

Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

Although I take @CirnoT's point that if we want order we should use a list - I suspect that we need to do this to keep compatibility with GH.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 6, 2020
@techknowlogick
Copy link
Member

Like @CirnoT I also would prefer the built-in marshal, and that even though Github's list is sorted by us doing it then that creates a guarantee that the first elem will always be primary, however Github doesn't state that as a promise in their documentation. So I don't think we should make that promise either to encourage devs working with API to not take data at face value.

If we put it in without the sorting, then later we decide to presort it, then that wouldn't be a breaking change, but if we start out sorting the list and decide to switch that would be breaking.

Otherwise, this PR LGTM and so I won't block it. Sidenote: @lafriks I can click "update branch" are you able to check "accept edits from maintainers"?

@lafriks
Copy link
Member Author

lafriks commented Jun 6, 2020

I can't seem to find such option for existing PR :)

@6543
Copy link
Member

6543 commented Jun 7, 2020

Ping bot

@lafriks lafriks merged commit 2874ab5 into go-gitea:master Jun 7, 2020
@lafriks lafriks deleted the feat/langstat_api branch June 7, 2020 11:48
@lafriks lafriks added type/enhancement An improvement of existing functionality and removed type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels Jun 7, 2020
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
* Add language statistics API

* Add tests
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] [API] export repo-language info
6 participants