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

refactor: rename bigquery source type #203

Merged
merged 4 commits into from
Feb 14, 2021

Conversation

AdriSolid
Copy link
Contributor

@AdriSolid AdriSolid commented Feb 12, 2021

Rename BigQuery source type, from bq to bigquery

For: https://app.clubhouse.io/cartoteam/story/135148/rename-source-type-in-template

Related with: CartoDB/carto-react#97

@vercel
Copy link

vercel bot commented Feb 12, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/carto-frontend/cra-template-carto/j6r0mspaf
✅ Preview: https://cra-template-c-git-refactor-ch135148rename-bigquery-sour-df1193.carto-frontend.vercel.app

@shortcut-integration
Copy link

This pull request has been linked to Clubhouse Story #135148: rename source type in template.

@@ -5,7 +5,7 @@ const { promptArgs, readFile, getFiles } = require('../../promptUtils');

const VIEWS_DIR = 'components/views'

const SOURCE_TYPES = ['sql', 'bq'];
const SOURCE_TYPES = ['sql', 'bigquery'];
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, if we have defined a type in the lib, we should export it and use them here, instead of a duplicated string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, didn't want to make this little 'breaking-change', going to add this then

@@ -56,7 +56,7 @@ const prompt = async ({ prompter, args }) => {

// Detect what kind of layer we need (CartoSQLLayer, CartoBQTilerLayer)
const selectedSourceFileContent = readFile(`src/data/sources/${answers.source_file}.js`);
const res = /(?:type: ')(?<type>sql|bq*)(?:')/g.exec(selectedSourceFileContent);
const res = /(?:type: ')(?<type>sql|bigquery*)(?:')/g.exec(selectedSourceFileContent);
Copy link
Contributor

Choose a reason for hiding this comment

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

If you make this dynamic, using the lib enum, you can use something like https://stackoverflow.com/questions/5090103/javascript-regexp-dynamic-generation-from-variables

And please double check the resulting regex, we already had an issue with this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, cannot require npm packages here, will wait for the creation of the lib cdn.

Copy link
Contributor

@VictorVelarde VictorVelarde Feb 12, 2021

Choose a reason for hiding this comment

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

I think we could just require from local node_modules with a relative path (from template) ... I'm gonna have a look

Copy link
Contributor

Choose a reason for hiding this comment

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

...not so direct, so we'll skip this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems a bit weird requiring a relative path from node_modules..

Copy link
Contributor

@VictorVelarde VictorVelarde left a comment

Choose a reason for hiding this comment

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

👍🏻

@VictorVelarde VictorVelarde merged commit 7da84ea into master Feb 14, 2021
@VictorVelarde VictorVelarde deleted the refactor/ch135148/rename-bigquery-source-type branch February 14, 2021 18:13
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