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

api design: tweaks to naming and get-lists-of-ids #1

Merged
merged 1 commit into from May 28, 2019

Conversation

Projects
None yet
3 participants
@ziz
Copy link
Collaborator

commented May 26, 2019

Add more "get a list of [x]" to the API, so the admin interface can
depend on the controller rather than hardcoding / knowing how to read
the sculpture configuration

Rename some REST paths in the design based on the admin controller
routes

@cswales
Copy link
Contributor

left a comment

I'm good with this, but I'll comment that the one thing that putting the controllerid in the URL doesn't easily give you is the ability to global change settings. (In the original case, for a global change you'd simply leave off the controllerid.) I honestly don't care which way we go with this (I actually went back and forth a couple times as I did the first write up, and finally decided just to go with something and see what people thought.)
Hmmm. Also not sure what the advantage of renaming 'volume_settings' to presets - did you have some idea of presets other than volume?

Thanks for doing this!

@cstigler

This comment has been minimized.

Copy link
Collaborator

commented May 27, 2019

@cswales to be clear, I think I'm the one who made those API changes you're talking about! I tweaked/changed some routes as I put them into our actual live route table (https://github.com/FlamingLotusGirls/Serenity/blob/master/admin/routes/api.js). I think @ziz saw that they were out of sync with design.txt (my bad!) and just updated design.txt to match my changes. To avoid them getting out of sync in the future, how about we just remove the routes from design.txt and only update them in the actual route table? No need to keep the same list in two places now.

  • re adding a "get list of" route: cool with me!

  • re putting the controller ID in the URL: either way's fine with me, I just had to change that route because putting the volume number in the URL (/sound/<soundControllerId>/volume/<volumelevel... 0-100>) hurt my precious REST purist soul :p

  • re "volume_settings" vs "presets": I don't have strong feelings about the word "presets", but I did find volume_settings kinda confusing because it doesn't imply saving / presetting the volume setting. It took me a couple reads to understand the difference from the /sound/volume route, since that also changes the volume setting (but in a different sense). If we're not into presets, how about volumePresets or savedVolumeLevels or similar?

Naming things is always such a hassle...

@ziz

This comment has been minimized.

Copy link
Collaborator Author

commented May 27, 2019

how about we just remove the routes from design.txt and only update them in the actual route table?

I was considering doing that here, but I decided that syncing the design first seemed like a more atomic change (and less likely to introduce transcription errors), so I'm in favor of that.

re putting the controller ID in the URL

I explicitly take no stance on REST purity; I'm a dogmatic pragmatist. So, my big question looking at this is: does a single controller only handle one sound effect at once, or can they be layered? (And if layered, do we get to mix volume levels at a sound-effect level or just at a controller level?)

re "volume_settings" vs "presets"

Seconded that volume_settings does not immediately imply "a collection of saved volume levels" to me. (Also it's not entirely clear if this stores active sound effects or just volume levels?)

@cswales

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

@cswales

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

@ziz

This comment has been minimized.

Copy link
Collaborator Author

commented May 27, 2019

Anyone else want to take a crack at [sound API] functionality?

Yeah, that sounds like a blast. I will write up some preliminary thoughts based on what you've said (and other resources if you can point me at them) in another PR. Probably for this PR we just shrug and recognize we are changing sound API a lot?

@cswales

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

Yep. Good with me.

@ziz

This comment has been minimized.

Copy link
Collaborator Author

commented May 28, 2019

Noted the expected sound API changes. Otherwise ready for merge, I think?

@cstigler

This comment has been minimized.

Copy link
Collaborator

commented May 28, 2019

@ziz LGTM! 🚢

(to be very nitpicky, I will note that the change volume level route in design.txt appears to have been typo'd - you wrote PUT /sound/<soundControllerId>/volume/<volumelevel... 0-100> while the route table has just /sound/:controllerId/volume which I think is prettier. That said, IDGAF because as noted I think we should remove routes from design.txt anyway!)

api design: tweaks to naming and get-lists-of-ids
Add more "get a list of <x>" to the API, so the admin interface can
depend on the controller rather than hardcoding / knowing how to read
the sculpture configuration

Rename some REST paths in the design based on the admin controller
routes

Note expected changes to sound API

@ziz ziz force-pushed the ziz:cleanup branch from 68a5cdd to f751afb May 28, 2019

@ziz

This comment has been minimized.

Copy link
Collaborator Author

commented May 28, 2019

Ah, good catch, which I've fixed while squashing (and which fix will go away when design.txt doesn't have routes (but I want to ask about implementing this API once (in python) versus twice (in python and in node) before we do too much more there).

Merge away.

@cstigler

This comment has been minimized.

Copy link
Collaborator

commented May 28, 2019

related question (especially for @cswales since she is the queen of this repo!):

what PR/review processes do we want to follow for development? my thought was: PRs preferred for non-typo changes so we have visibility and a chance for comment, but authors can feel free to merge at any time (without review) using their own judgment.

which is to say, @ziz maybe you can go ahead and merge yourself?

@ziz

This comment has been minimized.

Copy link
Collaborator Author

commented May 28, 2019

Glad to; just need access. (And that process seems fine to me.)

@ziz

This comment has been minimized.

Copy link
Collaborator Author

commented May 28, 2019

(oh, I found the problem: @cswales, you sent the collaboration invite to the github user I don't use)

@cswales

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

@ziz ziz merged commit 535beb0 into FlamingLotusGirls:master May 28, 2019

@ziz ziz deleted the ziz:cleanup branch May 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.