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

Variables State #210

Merged
merged 9 commits into from Sep 1, 2018

Conversation

Projects
None yet
3 participants
@ephread
Copy link
Contributor

commented Aug 30, 2018

This PR completes #193.

Thanks for reviewing!

NQNStudios and others added some commits Jul 30, 2018

Almost done with first pass VariablesState conversion
Co-authored-by: Eliana Abraham <elabraha@umich.edu>
Done with first pass of VariablesState
Co-authored-by: Eliana Abraham <elabraha@umich.edu>
Done with first pass of VariablesState
Co-authored-by: Eliana Abraham <elabraha@umich.edu>
Replacing Object assign with Map copy sonstructor
Co-authored-by: Eliana Abraham <elabraha@umich.edu>

@y-lohse y-lohse referenced this pull request Aug 31, 2018

Closed

WIP porting VariablesState #193

@y-lohse
Copy link
Owner

left a comment

Some questions but otherwise cool 👍

return this._batchObservingVariableChanges;
}
set batchObservingVariableChanges(value: boolean){
value = !!value; // TODO: Might have become useless with TypeScript.

This comment has been minimized.

Copy link
@y-lohse

y-lohse Aug 31, 2018

Owner

Yup, useless now.

get callStack(){
return this._callStack;
}
set callStack(callStack: any /* CallStack */){

This comment has been minimized.

Copy link
@y-lohse

y-lohse Aug 31, 2018

Owner

Why can't we use the CallStack type here ?

This comment has been minimized.

Copy link
@ephread

ephread Sep 1, 2018

Author Contributor

Definitely, I guess that Callstack wasn't merged yet when the PR was first worked on.

this._callStack = callStack;
this._listDefsOrigin = listDefsOrigin;

// TODO should we discourage the use of proxies since they're non-portable

This comment has been minimized.

Copy link
@y-lohse

y-lohse Aug 31, 2018

Owner

Can you elaborate more on this please?

This comment has been minimized.

Copy link
@ephread

ephread Sep 1, 2018

Author Contributor

I don't know much about proxies, this is most likely a comment inserted by @NQNStudios, so you'd have to ask them!

set(target: any, name, value){
if (name in target) target[name] = value;
else target.$(name, value);
return true; // returning a falsy value make the trap fail

This comment has been minimized.

Copy link
@y-lohse

y-lohse Aug 31, 2018

Owner

I wonder what I meant by that, but it doesn't look very sound.

This comment has been minimized.

Copy link
@ephread

ephread Sep 1, 2018

Author Contributor

Maybe we should rework the entire proxy thing later?

This comment has been minimized.

Copy link
@y-lohse

y-lohse Sep 1, 2018

Owner

Yah, we'll see :)

@ephread

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2018

With the updates, the build is successful locally but fails on Travis, let me investigate.

@y-lohse

This comment has been minimized.

Copy link
Owner

commented Sep 1, 2018

Awesome, thanks for the last changes!

@y-lohse y-lohse merged commit 8f1a069 into y-lohse:typescript Sep 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

y-lohse added a commit that referenced this pull request Sep 14, 2018

feat: port variables state (#210)
* Almost done with first pass VariablesState conversion

Co-authored-by: Eliana Abraham <elabraha@umich.edu>

* Done with first pass of VariablesState

Co-authored-by: Eliana Abraham <elabraha@umich.edu>

* Done with first pass of VariablesState

Co-authored-by: Eliana Abraham <elabraha@umich.edu>

* Replacing Object assign with Map copy sonstructor

Co-authored-by: Eliana Abraham <elabraha@umich.edu>

* style: Fix linter warnings

* fix: Reorganize file to match C# order and fix map copy

* fix: Replace any by CallStack where it makes sense

* fix: remove useless boolean coercion

* fix: Explicitely state return type of varValue

y-lohse added a commit that referenced this pull request Sep 14, 2018

feat: port variables state (#210)
* Almost done with first pass VariablesState conversion

Co-authored-by: Eliana Abraham <elabraha@umich.edu>

* Done with first pass of VariablesState

Co-authored-by: Eliana Abraham <elabraha@umich.edu>

* Done with first pass of VariablesState

Co-authored-by: Eliana Abraham <elabraha@umich.edu>

* Replacing Object assign with Map copy sonstructor

Co-authored-by: Eliana Abraham <elabraha@umich.edu>

* style: Fix linter warnings

* fix: Reorganize file to match C# order and fix map copy

* fix: Replace any by CallStack where it makes sense

* fix: remove useless boolean coercion

* fix: Explicitely state return type of varValue

y-lohse added a commit that referenced this pull request Sep 16, 2018

feat: port variables state (#210)
* Almost done with first pass VariablesState conversion

Co-authored-by: Eliana Abraham <elabraha@umich.edu>

* Done with first pass of VariablesState

Co-authored-by: Eliana Abraham <elabraha@umich.edu>

* Done with first pass of VariablesState

Co-authored-by: Eliana Abraham <elabraha@umich.edu>

* Replacing Object assign with Map copy sonstructor

Co-authored-by: Eliana Abraham <elabraha@umich.edu>

* style: Fix linter warnings

* fix: Reorganize file to match C# order and fix map copy

* fix: Replace any by CallStack where it makes sense

* fix: remove useless boolean coercion

* fix: Explicitely state return type of varValue

y-lohse added a commit that referenced this pull request Sep 23, 2018

feat: port variables state (#210)
* Almost done with first pass VariablesState conversion

Co-authored-by: Eliana Abraham <elabraha@umich.edu>

* Done with first pass of VariablesState

Co-authored-by: Eliana Abraham <elabraha@umich.edu>

* Done with first pass of VariablesState

Co-authored-by: Eliana Abraham <elabraha@umich.edu>

* Replacing Object assign with Map copy sonstructor

Co-authored-by: Eliana Abraham <elabraha@umich.edu>

* style: Fix linter warnings

* fix: Reorganize file to match C# order and fix map copy

* fix: Replace any by CallStack where it makes sense

* fix: remove useless boolean coercion

* fix: Explicitely state return type of varValue

y-lohse added a commit that referenced this pull request Sep 23, 2018

feat: port variables state (#210)
* Almost done with first pass VariablesState conversion

Co-authored-by: Eliana Abraham <elabraha@umich.edu>

* Done with first pass of VariablesState

Co-authored-by: Eliana Abraham <elabraha@umich.edu>

* Done with first pass of VariablesState

Co-authored-by: Eliana Abraham <elabraha@umich.edu>

* Replacing Object assign with Map copy sonstructor

Co-authored-by: Eliana Abraham <elabraha@umich.edu>

* style: Fix linter warnings

* fix: Reorganize file to match C# order and fix map copy

* fix: Replace any by CallStack where it makes sense

* fix: remove useless boolean coercion

* fix: Explicitely state return type of varValue

@ephread ephread deleted the ephread:VariablesState branch Sep 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.