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

style: prefer const #2820

Merged
merged 1 commit into from Aug 31, 2020
Merged

style: prefer const #2820

merged 1 commit into from Aug 31, 2020

Conversation

domoritz
Copy link
Member

@domoritz domoritz commented Aug 18, 2020

There are still many places where we can't use const because multiple variables are defined with a single var/let but we can fix those later.

Maybe we can run a codemod (https://github.com/facebook/codemod) to upgrade all of the code automatically. But at the same time we should probably switch to prettier and babel.

@domoritz
Copy link
Member Author

cc @lgrammel

@lgrammel
Copy link
Contributor

Hey! Maybe some background on my pull requests, it was a dry run for an automated tool (Jeff suggested trying it out on Vega). It's similar to codemods. I could actually implement a split of let statements that define multiple variables, some of which could be const if there's interest?

@domoritz
Copy link
Member Author

Awesome. Yeah, I would love to have a more consistent code style between the different Vega projects.

I recently submitted a pull request to d3 scales to use let/const (d3/d3-scale#212). The other d3 repos still need to be updated (d3/d3#3435) so there may be interest beyond Vega for your tool.

@lgrammel
Copy link
Contributor

Cool, thanks for the pointer. I'll see if this works for the d3 repos as well.

@domoritz domoritz requested a review from a team August 19, 2020 14:06
@lgrammel
Copy link
Contributor

@domoritz I've run 3 codeshifts (var -> let/const, arrow functions, object property shorthands) that would result in roughly ~500 changes. Is this something it would make sense for me to create PRs for? And if so, how would those PRs ideally be sized (e.g. per file/codeshift, for all files at once (one PR per codeshift), some intermediate slicing (e.g. per directory)?

@domoritz
Copy link
Member Author

I'd be happy with one pull request for everything since all the changes are of the same kind. But I'll defer to @jheer for deciding on what goes into master.

@domoritz
Copy link
Member Author

Also see #2827

@jheer jheer merged commit 1b57a75 into master Aug 31, 2020
@jheer jheer deleted the dom/const branch August 31, 2020 10:54
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.

None yet

3 participants