Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

Test cases for js-ast-utils/is-binary #1047

Merged
merged 3 commits into from Aug 12, 2020
Merged

Conversation

nt591
Copy link
Contributor

@nt591 nt591 commented Aug 12, 2020

Part of #1023

Summary

This PR adds test cases for the isBinary utility checking binary and non-binary expressions (unary, arrays).

Note: I wasn't sure if the copyright header was needed but I saw it in other tests I used as samples, so I kept it. Please let me know if it needs to change.

Test Plan

It runs and passes with ./rome test internal/js-ast-utils/isBinary.test.ts

No changes to other tests, so ./rome test still passes (I've verified)

Signed-off-by: Nikhil Thomas <nikhil@arceoanalytics.com>
Copy link
Contributor

@yassere yassere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! The copyright header isn't necessary on any new files. We only have to keep it on files that already have it.

@nt591
Copy link
Contributor Author

nt591 commented Aug 12, 2020

Thanks for working on this! The copyright header isn't necessary on any new files. We only have to keep it on files that already have it.

Awesome! Is "isn't necessary" the same as "please delete" or "next time don't worry about it" - I'm happy to add a change, just want to make sure I'm doing the copyright-thing right

@yassere
Copy link
Contributor

yassere commented Aug 12, 2020

Yes, please delete the header.

Also, I think it's better to use jsExpressionStatement.assert() instead of an as assertion and to use t.true() and t.false() instead of t.inlineSnapshot() when you're just checking a boolean value.

Signed-off-by: Nikhil Thomas <nikhil@arceoanalytics.com>
import {parseJS} from "@internal/js-parser";
import {jsExpressionStatement} from "@internal/ast";

function binaryExpressionHelper(input: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function binaryExpressionHelper(input: string) {
function binaryExpressionHelper(input: string): boolean {

Nit: Explicit return types where possible.

}

test(
"isBinary",
Copy link
Contributor

@ematipico ematipico Aug 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to give a better wording here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can be more explicit - I was looking at a few other tests (sort.test.ts, parse.test.ts) and didn't see very lengthy test names

Signed-off-by: Nikhil Thomas <nikhil@arceoanalytics.com>
@bitpshr bitpshr merged commit 5711a82 into rome:main Aug 12, 2020
@nt591 nt591 deleted the isBinary-tests branch August 12, 2020 21:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants