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 some tests and definition of dependencies option #164

Merged
merged 4 commits into from
Aug 17, 2021

Conversation

Mingun
Copy link
Member

@Mingun Mingun commented Jun 3, 2021

This PR fixes three things:

  • some new behavior tests did not use the options, so not all variants were tested
  • options.definitions was declared with type any instead of object
  • options.exportVar was declared with type any instead of string; for globals format it is required

@Mingun Mingun force-pushed the fixes branch 2 times, most recently from 136be26 to a4b54df Compare June 10, 2021 16:37
@Mingun
Copy link
Member Author

Mingun commented Jun 12, 2021

I think, this is next that should be merged. I'll rebase other my PR on this.

@hildjj
Copy link
Contributor

hildjj commented Jun 12, 2021

Add Dependencies to test/types/peg.test-d.ts?

@Mingun
Copy link
Member Author

Mingun commented Jun 14, 2021

I've looked to existent tests -- all of them check that something that returned or passed as parameter has a specified type. In that case situation is different -- we should pass object with type Dependencies to the method. I'm not sure:

  • what we want to check there?
  • how to do that correctly?

@hildjj
Copy link
Contributor

hildjj commented Aug 17, 2021

I'm going to merge this. I may go and add some ts tests myself, but let's do that in parallel so you can keep moving without having to continue to rebase this.

@hildjj hildjj merged commit 6fc2928 into peggyjs:main Aug 17, 2021
@Mingun Mingun deleted the fixes branch August 17, 2021 19:45
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.

2 participants