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

fix: typing of terminalObjectFunction (this) #896

Merged
merged 2 commits into from Aug 16, 2023
Merged

Conversation

antoineol
Copy link
Contributor

The PR instructions say to make the PR against the devel branch, but that branch seems behind master. I was thinking it would be simpler for you to target master for the merge :)

This PR is a super simple change in a typing, not to have typing errors in the hello world example from this article (type this).

@jcubic
Copy link
Owner

jcubic commented Aug 15, 2023

Thanks for the fix, it seems that I've forget the type for this in this particular type.

But please use devel branch. It will help me know that there are things to be released. I've update devel branch when I get back home. I forget to update it after merging devel to master before the release.

@jcubic
Copy link
Owner

jcubic commented Aug 15, 2023

Also there is a file that test the types, it also need to be updated. For instance to call this.echo in object interpreter.

@coveralls
Copy link

coveralls commented Aug 15, 2023

Coverage Status

coverage: 81.798%. remained the same when pulling df55072 on antoineol:master into 1195f78 on jcubic:devel.

@antoineol antoineol changed the base branch from master to devel August 16, 2023 05:28
@antoineol
Copy link
Contributor Author

I've added a test and the tsconfig flag that reveals the error. I would recommend strict: true instead, but it adds many more TS errors I don't have time to fix now :)

For the sake of keeping the current PR (instead of opening a new one), you will see the changes in the source repo (fork) in master branch, but I have force put it at the devel commit, so the diff should be good to merge the PR.

@jcubic jcubic merged commit 368b0f3 into jcubic:devel Aug 16, 2023
3 checks passed
@jcubic
Copy link
Owner

jcubic commented Aug 16, 2023

Thanks for the fix

@antoineol
Copy link
Contributor Author

Btw I haven't said earlier, but thank to you, first, for this amazing library! It's the simplest way I have found to code a fake terminal 👍

@jcubic
Copy link
Owner

jcubic commented Aug 26, 2023

I have no idea why but tests failed after merging. I need to investigate. Just found it now.

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