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

Misnamed key in configuration object for createEntityAdapter #370

Closed
karptonite opened this issue Sep 11, 2017 · 6 comments
Closed

Misnamed key in configuration object for createEntityAdapter #370

karptonite opened this issue Sep 11, 2017 · 6 comments

Comments

@karptonite
Copy link
Contributor

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Feature request
[ ] Documentation issue or request

This is a follow up for issues #367 and #369. While the documentation now indicates that the sort parameter is a compare function, it is still called sort In the configuration object, which could lead to confusion. It seems worth fixing the name before a beta of the entity package is actually released.

@karptonite karptonite changed the title Misnamed key in API for createEntityAdapter Misnamed key in configuration object for createEntityAdapter Sep 11, 2017
@brandonroberts
Copy link
Member

brandonroberts commented Sep 11, 2017

I think sort is clearer, because you either disable it or you provide a custom comparer function used for sorting. I would be more in favor of sort as a pure boolean value and adding an additional property for a custom comparer function.

@karptonite
Copy link
Contributor Author

Good point. I agree that if you call it compare then setting to false to leave it unsorted is confusing. But it is already optional, so you could leave it unset if you were not sorting. Under what circumstances would you ever want to set it explicitly to false? It seems that of you just had a sortCompare field, and sorted only if sortCompare is set, that may be clear for both usages.

@brandonroberts
Copy link
Member

You wouldn't need to explicitly set it to false unless you have it wrapped in some sort of factory. I'm leaning towards leaving it as is for now.

@karptonite
Copy link
Contributor Author

karptonite commented Sep 12, 2017

It is your call. I still think that calling the parameter sort, when what it takes is a custom comparer, is not correct, especially when there is an actual name for what you are passing. It isn't a terrible thing, but what if, down the road, you want to add that sort boolean that you mentioned after all? If you leave it as it is, the parameter name is taken by something that isn't actually sort. Especially if you don't expect it to be used as a boolean, it makes more sense to me to call it what it is.

@brandonroberts
Copy link
Member

Valid points. I think sortComparer is closer to describing what the key does. If its false, no sorting will be done, otherwise you provide a function that will be used for sorting.

@karptonite
Copy link
Contributor Author

I agree that sortComparer is a good name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants