Skip to content
This repository has been archived by the owner on Apr 3, 2022. It is now read-only.

Custom binIDs [WIP: Needs some frontend work still] #115

Closed
wants to merge 5 commits into from

Conversation

aranasaurus
Copy link
Member

Changes to support custom bin IDs for #82. Also makes changes that effectively implement the activity refreshing the expiration (#101) as any request to a bin that doesn't exist will create said bin instead of 404ing. We'll need to talk more about whether or not the data should actually persist more than 48 hours based on activity.

@aaronpk
Copy link
Contributor

aaronpk commented Nov 21, 2014

+1 to leaving it where it deletes data after 48 hours. This is not supposed to be a place to store data.

@aranasaurus
Copy link
Member Author

Custom bin IDs don't show up in the history currently. Making a note here so that I'll remember to fix that before marking this PR ready to merge.

@@ -13,6 +13,28 @@ import (
"github.com/nu7hatch/gouuid"
)

func (gb *geobinServer) createBin(n string, w http.ResponseWriter) (time.Time, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This function needs a doc comment!

…r the data visibility, expiration, and custom bin ids stuff.
@ungoldman
Copy link
Member

@aranasaurus is this good to merge?

@aranasaurus
Copy link
Member Author

I don't think so. See comments in #82 and #101 for what is left to do. Another gopher should look at it too, I feel like there was something broken in the way I did it, but I can't remember what now.

@ungoldman
Copy link
Member

well it would be merged into the Dev branch so we could at least continue development on it from here rather than just your fork

@aranasaurus
Copy link
Member Author

ok. I could also just push it to a branch upstream that's not dev. I think it was far enough along to not break everything though... so it's probably fine to merge to dev. CI says it's good, I guess.

In other words, either way. Merge it if you want, or just close the PR and I'll push the branch.

@ungoldman
Copy link
Member

@aranasaurus If you've got time to push it to a feature branch on upstream that works for me. I just want to help get it done and live. Talked with Court and Josh a bit about potentially decoupling web app from api (e.g. geobin.io & api.geobio.io repos) so we can develop them in parallel and not mix streams too much.

@aranasaurus
Copy link
Member Author

That sounds great. I hope to get some more time to put toward geobin again, hopefully next month...

Anyway, feature branch created!

@ungoldman
Copy link
Member

@aranasaurus Cool, I will close this PR and futz with your feature branch on my fork. Thanks!

@ungoldman ungoldman closed this Apr 4, 2015
@ungoldman
Copy link
Member

@aranasaurus going to rebase custom-binids off master & force push so the tests point to geobin-io instead of esripdx.

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

Successfully merging this pull request may close these issues.

3 participants