feat(binding): improve AST $kind classifying and other small tweaks/fixes #213
Conversation
…literals if they are pure
|
Code Climate has analyzed commit 87635f5 and detected 1 issue on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 91.2% (50% is the threshold). This pull request will bring the total coverage in the repository to 90.9% (0.4% change). View more on Code Climate. |
Codecov Report
@@ Coverage Diff @@
## master #213 +/- ##
==========================================
+ Coverage 90.08% 90.39% +0.31%
==========================================
Files 83 83
Lines 6744 6809 +65
Branches 1132 1139 +7
==========================================
+ Hits 6075 6155 +80
+ Misses 669 654 -15
Continue to review full report at Codecov.
|
|
This pull request fixes 2 alerts when merging 5bd30eb into 1e804a0 - view on LGTM.com fixed alerts:
Comment posted by LGTM.com |
|
@EisenbergEffect @bigopon This is just some leftover work in progress I had on the previous PR. It's ready now. Ish. I will finish up the AST tests later, getting a bit bored of it :) |
|
This pull request fixes 2 alerts when merging cc8bb7d into 1e804a0 - view on LGTM.com fixed alerts:
Comment posted by LGTM.com |
packages/runtime/src/binding/ast.ts
Outdated
| BindingIdentifier = 0b0010000000_10110, | ||
| ForOfStatement = 0b0000100000_10111, | ||
| Interpolation = 0b0000001000_11000 | ||
| Connects = 0b000000000001_00000, // The expression's connect() function is not a no-op |
There was a problem hiding this comment.
nit: Maybe highlight not by uppercasing it. very easy to miss this important signal
There was a problem hiding this comment.
Yeah agree. I just made the comment a bit more informative and consistent with the rest. "not a no-op" was a bit lackluster in info anyway
There was a problem hiding this comment.
While we are here, should we also try to make it consistent the way we want to have instance constant. Current its different between instruction and AST?
packages/runtime/src/binding/ast.ts
Outdated
| const $emptyObject = new ObjectLiteral(PLATFORM.emptyArray, PLATFORM.emptyArray); | ||
| const $emptyTemplate = new Template(['']); | ||
|
|
||
| Object.defineProperty(AccessThis, '$this', { value: $this, writable: false, enumerable: true, configurable: false }); |
There was a problem hiding this comment.
Well, maybe change the block to something like this: it looks too bulky
((dP, baseConfig) => {
baseConfig.value = ....
dP(Class, prop, baseCfg);
})(Object.defineProperty, { value: null, writeable: fasle, enumerable: true, configurable: false } );There was a problem hiding this comment.
On a second thought, do we need this setup? Wouldn't a normal assignment be enough?
There was a problem hiding this comment.
Yeah I can't really make up my mind on that either. I had static readonly ... = new ... first. I changed it because I had this vague worry about assigning them statically before the other prototype modifications down below were done. Not really any actual basis for that though.
There was a problem hiding this comment.
Okay I made up my mind. They're static properties now. I really don't see much point in defining them via reflection as they're already marked readonly.
fkleuver
changed the title
|
This pull request fixes 2 alerts when merging 8e5ae9e into 1e804a0 - view on LGTM.com fixed alerts:
Comment posted by LGTM.com |
|
This pull request fixes 2 alerts when merging 738e0f7 into 1e804a0 - view on LGTM.com fixed alerts:
Comment posted by LGTM.com |
|
This pull request fixes 2 alerts when merging 33d9431 into 1e804a0 - view on LGTM.com fixed alerts:
Comment posted by LGTM.com |
|
This pull request fixes 2 alerts when merging 87635f5 into 1e804a0 - view on LGTM.com fixed alerts:
Comment posted by LGTM.com |
|
Very nice! |
No description provided.