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

Allow accessing fetched entities of a Storage #515

Merged
merged 1 commit into from
Nov 22, 2018
Merged

Conversation

torkleyy
Copy link
Member

@torkleyy torkleyy commented Nov 22, 2018

I've added Storage::entities to allow making extension methods for Storage that return Entity.

This method is hidden because I'm 100% certain newcomers will be confused by this otherwise. This makes it less visible, yes, but I think that's worth for avoiding this confusion.


This change is Reviewable

@torkleyy torkleyy added the ready label Nov 22, 2018
@Xaeroxe
Copy link
Member

Xaeroxe commented Nov 22, 2018

I don't think it's worth it to hide this without demonstrated evidence it confuses people. I think we should leave it visible until we get a lot of confused users.

@torkleyy
Copy link
Member Author

torkleyy commented Nov 22, 2018

Hmm, I thought you're going to say something like this ^^
The problem is simply that users tend to use features that are more complicated than what they actually need. The fact that a storage fetches the entities resource is not known to many users.

That's why I hid it. If you insist on this, the only option I see is to give it a longer name to make sure this is obvious.

@Xaeroxe
Copy link
Member

Xaeroxe commented Nov 22, 2018

I'd definitely be in favor of the longer name option. The way it is currently would still come up in editor auto completes but there would be no documentation. That's even more confusing.

@torkleyy
Copy link
Member Author

Done.

Copy link
Member

@Xaeroxe Xaeroxe left a comment

Choose a reason for hiding this comment

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

Perfect, thanks!

bors r+

bors bot added a commit that referenced this pull request Nov 22, 2018
515: Allow accessing fetched entities of a `Storage` r=Xaeroxe a=torkleyy

I've added `Storage::entities` to allow making extension methods for `Storage` that return `Entity`.

This method is hidden because I'm 100% certain newcomers will be confused by this otherwise. This makes it less visible, yes, but I think that's worth for avoiding this confusion.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/slide-rs/specs/515)
<!-- Reviewable:end -->


Co-authored-by: Thomas Schaller <torkleyy@gmail.com>
Copy link
Contributor

@AnneKitsune AnneKitsune left a comment

Choose a reason for hiding this comment

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

Double approval, you are lucky :3

@bors
Copy link
Contributor

bors bot commented Nov 22, 2018

Build succeeded

@bors bors bot merged commit 1a730dc into amethyst:master Nov 22, 2018
@torkleyy torkleyy deleted the ent branch November 22, 2018 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants