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

Meaningful name to represent 1000 #948

Closed
at758 opened this issue Sep 7, 2018 · 7 comments
Closed

Meaningful name to represent 1000 #948

at758 opened this issue Sep 7, 2018 · 7 comments

Comments

@at758
Copy link

at758 commented Sep 7, 2018

💥 Proposal

Can we add a meaningful name to represent what 1000 is?

Have you read the Contributing Guidelines on issues?

Can we add a meaningful name for what 1000 is, its not super clear why the check (i++ < 1000) is needed and also this function seems to be doing a lot of things. Would be better to split this out into multiple functions

https://github.com/facebook/Docusaurus/blob/master/lib/server/readCategories.js#L89

@endiliey
Copy link
Contributor

@JoelMarcey ?

@JoelMarcey
Copy link
Contributor

I believe we chose this as an arbitrary number to limit the number of possible categories. 1000 seemed like a good upper limit before things just got unwieldy. 😄

@deltice -- do you recall if 1000 had greater significance here?

@at758
Copy link
Author

at758 commented Sep 10, 2018

Thanks, @JoelMarcey would be helpful to add a comment there or have a name that is good enough to communicate this message along.

CATEGORY_COUNT_THRESHOLD maybe?

@JoelMarcey
Copy link
Contributor

Sure. We can do that. Would you like to submit a PR @at758 ?

@at758
Copy link
Author

at758 commented Sep 10, 2018

Sure I can do that.

@endiliey
Copy link
Contributor

Upon seeing at the code closely, best if we re-write this. This is a matter of building categories from all the docs metadata.

Current implementation is inefficient.. quite surprised it's been there since initial commit.

Cc @yangshun

@deltice
Copy link
Contributor

deltice commented Sep 19, 2018

Yeah, I think this was just arbitrarily chosen and I may have forgotten about it as I continued working on the features. 😬 I don't believe there's any special about it.

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

No branches or pull requests

4 participants