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

Group endpoints #360

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Group endpoints #360

wants to merge 1 commit into from

Conversation

dklmuc
Copy link
Contributor

@dklmuc dklmuc commented Nov 13, 2017

Relates to #299 (comments)

Group endpoints in dashboard and docs for better clarity

taffy_grouped_dashboard

@atuttle
Copy link
Owner

atuttle commented Nov 13, 2017

Reading through the code this looks great. Thanks, @dklmuc! I would like to pull it down locally and play with it a bit before merging though, so it will have to wait until I have some time to do that. Really appreciate the effort!

@dklmuc
Copy link
Contributor Author

dklmuc commented Nov 13, 2017

no problem, we have our customized version (which I'm trying to bring back to an updateable state) ;-)

Also already thought about making this configurable per setting, so I'm open to change requests.

@atuttle
Copy link
Owner

atuttle commented Nov 13, 2017

I could see it being somewhat annoying if there's only 1 thing in every "group". Maybe this should only kick in after there are > N resources? I don't know what that number should be.

@dklmuc
Copy link
Contributor Author

dklmuc commented Nov 13, 2017

Imho I think a boolean setting would be best if someone didn't want to use it (or make it default false).

We have got an API here with 17 first level "groups", some of them with up to 30 endpoints and some of them with only 3 or less endpoints. Which looks great also when some of the small "groups" coming one after another. I don't think a mixture depending on the count would add benefits here.

@atuttle
Copy link
Owner

atuttle commented Nov 13, 2017

I wasn't proposing a mixture, just avoiding the boolean config by turning it on once there are more than >N total resources. (It would be a bit overkill if your API has 2-3 resources)

@dklmuc
Copy link
Contributor Author

dklmuc commented Nov 13, 2017

I think in the end somebody will ask for lowering the defined value, then somebody will come and ask to raise it again. So finally it will become a configurable setting to group on higher ressource counts.

Personally I would set it to zero, also for small APIs, therefor I've thought about just on/off.

@atuttle
Copy link
Owner

atuttle commented Nov 14, 2017

One of the values I tried to bake into Taffy was that configuration is evil. You can't get completely away from it, but in most cases simply letting the framework "have an opinion" is enough to eliminate proposed config.

I think this is a good addition and if we want it to be zero-config (which I do) then I think always-on is the right way to go.

@bosonau
Copy link

bosonau commented Jan 8, 2020

Any idea if or when this will be merged to master? I'd love to use this feature with our current REST api and would love it if it's in master as is

@atuttle
Copy link
Owner

atuttle commented Aug 3, 2020

I'd love to add it -- I manage an API with several dozen resources too and this would be a great addition. I still think we want to add a config setting to either enable/disable this behavior, or set the threshold for minimum total # of resources before it turns on. I don't have a strong preference for one or the other of those implementations, but whichever we go with I'd like to see it default to turned on (so if it's the minimum total, default to 0).

@dklmuc
Copy link
Contributor Author

dklmuc commented Aug 16, 2020

@atuttle With the merged PR #394 what's the preference now regarding output?

just a groupname like in screen at top and then full uri for all resources in this group like:

message
/messages/messages
/messages/message

or splitting the uri like:

/messages/
messages
message

@atuttle
Copy link
Owner

atuttle commented Oct 1, 2020

@dklmuc my preference would be to always include the full URI.

/messages
/messages/messages
/messages/message

@atuttle atuttle added the Semver-Minor This change will necessitate a minor version bump label Jan 14, 2022
@atuttle
Copy link
Owner

atuttle commented Jan 14, 2022

Checking in @dklmuc. I'd still like to get this landed.

@psarin
Copy link

psarin commented Feb 13, 2024

This seems like a great feature. @atuttle : If @dklmuc doesn't have the bandwidth right now, I can take a look at it. Can you let me know which part(s) from discussion above still need to be worked on? 1. the zero-config aspect with n>X triggering it and/or 2. the specific output of the groupnames / subitem URIs? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Semver-Minor This change will necessitate a minor version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants