-
Notifications
You must be signed in to change notification settings - Fork 437
Update tokenizer to use Moo instead #224
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
Changes from all commits
23cdb66
db38b66
bc88e99
a9dd035
a80cfc8
9473e19
39e4989
755ffb0
9117964
7a8cc4d
e863898
79ed1f7
c1d4150
dca3a8f
5509e31
bbeb179
ba2d7a8
f63f78d
c7ce114
bec0ac7
5201f7c
a2f4e05
cb5a6c9
87f786f
436c162
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,8 @@ | ||
| { | ||
| "presets": ["@babel/preset-env", "@babel/preset-typescript"], | ||
| "plugins": ["@babel/plugin-proposal-class-properties", "add-module-exports"] | ||
| "plugins": [ | ||
| "@babel/plugin-proposal-class-properties", | ||
| "add-module-exports", | ||
| ["babel-plugin-inline-import", { "extensions": [".sql"] }] | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,8 @@ | ||
| import Formatter from '../core/Formatter'; | ||
| import Tokenizer from '../core/Tokenizer'; | ||
| import type { StringPatternType } from '../core/regexFactory'; | ||
| import { EOF_TOKEN, Token } from '../core/token'; | ||
| import { dedupe } from '../utils'; | ||
| import Formatter from 'src/core/Formatter'; | ||
| import Tokenizer from 'src/core/Tokenizer'; | ||
| import { type StringPatternType } from 'src/core/regexFactory'; | ||
| import { EOF_TOKEN, type Token } from 'src/core/token'; | ||
| import { dedupe } from 'src/utils'; | ||
|
|
||
| /** | ||
| * Priority 5 (last) | ||
|
|
@@ -835,8 +835,8 @@ export default class BigQueryFormatter extends Formatter { | |
| reservedBinaryCommands, | ||
| reservedDependentClauses, | ||
| reservedKeywords: dedupe([ | ||
| ...Object.values(reservedFunctions).reduce((acc, arr) => [...acc, ...arr], []), | ||
| ...Object.values(reservedKeywords).reduce((acc, arr) => [...acc, ...arr], []), | ||
| ...Object.values(reservedFunctions).reduce((acc, arr) => acc.concat(arr), []), | ||
| ...Object.values(reservedKeywords).reduce((acc, arr) => acc.concat(arr), []), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this whole "replace spread with concat"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whether there's any speed difference really comes down to how does a browser or NodeJS implement the spread operator. Ideally these run at the exact same speed, like Babel will compile the spread to concat when targeting platforms without spread support. As always with performance, we should actually measure to see whether there is any practical difference in our case. IMHO a larger problem is that we're repeating this |
||
| ]), | ||
| stringTypes: BigQueryFormatter.stringTypes, | ||
| indexedPlaceholderTypes: ['?'], | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why warnings instead of errors?
I have found that it's better to not have the middle-ground that "warn" provides. It raises a question of if it's only a warning then should it be fixed or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this rule in particular blocks tests from being run if lines are partially commented, it should eventually be addressed before being merged in but while work is in progress, it's mostly a hindrance