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

Use pecan module instead of prior code from cartodb.js #4999

Merged
merged 13 commits into from
Aug 17, 2015
Merged

Conversation

viddo
Copy link
Contributor

@viddo viddo commented Aug 12, 2015

Fixes #4999

Since extracting pecan-specific code to an external module, use this new module as dependency instead (of cartodb.js where the code were located previously).

Some details:

  • The _analyzeStats method (and related code) was also extracted to pecan module, to keep "magic numbers" in one place (see Replace internal magic numbers w/ optional variables for guessMap pecan#1 for motivation/planned work related to this)
  • Changed the guessMap method signature slightly while I extracted the code, due to:
    • only ask for what's really needed
    • remove notion of Backbone in the pecan code
    • make the underscore dependency explicit and to be provided from caller (i.e. this repo), since that dependency is fixed and used from cartodb.js
  • Set laxed dependency to v1.x to automatically get minor and patch updates on npm install (e.g. deploys), since they should be backward compatible. This will allow us to update pecan code w/o have to jump through all the hoops of creating code changes in this repo every time.

Verified working locally, deployed on staging ded03 if you want to have a look yourself, should work just like before.

Old code will be removed from cartodb.js after this is merged and deployed.

ping @javisantana @javierarce @fdansv
cc @ohasselblad

Nicklas Gummesson added 8 commits August 11, 2015 15:26
Not needed, append to response object since that’s where it’s wanted
Only need to provide the minimum necessary and don’t for the Pecan
library to adhere to how CartoDB code is structured/named stuff
@viddo
Copy link
Contributor Author

viddo commented Aug 13, 2015

Bump! (and re-deployed on ded03 due to update w/ master)

@javierarce
Copy link
Contributor

👍 LGTM

@viddo
Copy link
Contributor Author

viddo commented Aug 13, 2015

@javierarce apart from looking at the code, tested on staging too? You guys know these parts better than I do and I'd like to make sure we're not breaking anything when releasing this. Do we have some smoke tests for Pecan?

@javierarce
Copy link
Contributor

No, sorry, I'll have a look to staging now.

I don't think we have smoke tests for Pecan. I'll add a ticket to add some tests.

@javierarce
Copy link
Contributor

Ok, I've tested it in staging using datasets from the Common Library and it seems to behave OK.

viddo added a commit that referenced this pull request Aug 17, 2015
Use pecan module instead of prior code from cartodb.js
@viddo viddo merged commit e6836fd into master Aug 17, 2015
@viddo viddo deleted the pecan-module branch August 17, 2015 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants