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

Make timestamp param position consistent. Add some tests #1

Merged
merged 4 commits into from Nov 17, 2014

Conversation

Projects
None yet
2 participants
@nberger
Copy link
Contributor

commented Nov 17, 2014

No description provided.

nberger added some commits Nov 17, 2014

Make timestamp param position to be consistent
In some places it was before id, in others it was after the id.

Makes it always to be placed before the id param
Add some tests for add-fact! & add-dimension!
Adds drop-schema to TimeSeries. It's needed to cleanup the db in tests
@@ -20,7 +20,7 @@
information for the fact on all the specified categories")

(inc! [service id categories]
[service id timestamp categories]
[service timestamp id categories]

This comment has been minimized.

Copy link
@nberger

nberger Nov 17, 2014

Author Contributor

@guilespi I'm changing the params order here, to make it consistent with new-fact params order. What do you think?

@guilespi

This comment has been minimized.

Copy link

commented on src/time_series_storage/postgres.clj in dffee95 Nov 17, 2014

En realidad acá te falta iterar sobre todas las tablas de counters definidas y dropearlas.

Acá estas bajando las tablas de definición pero no las tablas de datos propiamente dichas.

This comment has been minimized.

Copy link
Owner Author

replied Nov 17, 2014

Si, pero todavía no necesité eso para los tests que escribí hasta ahora. Lo pensaba hacer cuando avance con los tests que insertan facts

@guilespi

This comment has been minimized.

Copy link

commented on 203e6c6 Nov 17, 2014

Me parece bien el cambio pero preferiría que fuera al reves, primero id y después timestamp.

Asumiendo eso que el id tiene más peso y prefiero ver:

(inc! :conversions now)

a

(inc! now :conversions)

no estoy incrementando ahora las conversiones, sino que estoy incrementando las conversiones ahora :)

This comment has been minimized.

Copy link
Owner Author

replied Nov 17, 2014

Tenés razón, lo cambio.

nberger added some commits Nov 17, 2014

Put timestamp after id
The id is more important than the timestamp, so it should appear first
@nberger

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2014

@guilespi: Changed the params order, so id appears before timestamp. How does it look now?

guilespi added a commit that referenced this pull request Nov 17, 2014

Merge pull request #1 from nberger/make-timestamp-position-consistent
Make timestamp param position consistent. Add some tests

@guilespi guilespi merged commit e1725e4 into guilespi:master Nov 17, 2014

@nberger nberger deleted the nberger:make-timestamp-position-consistent branch May 12, 2015

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.