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: 209 Calling new Array with 1 argument doesn't work properly #233

Closed
wants to merge 4 commits into from
Closed

fix: 209 Calling new Array with 1 argument doesn't work properly #233

wants to merge 4 commits into from

Conversation

cisen
Copy link
Contributor

@cisen cisen commented Jan 23, 2020

ew Array with 1 argument doesn't work properly
.writable(true)
.configurable(false)
.enumerable(false);
let length = if args.len() > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can extract just the length and write the logic for creating the property once, e.g.

let n = if args.len() > 1 { args.len() } ...

Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

LGTM! Maybe a match statement would be a better fit instead of the if statements and would also allow you to recapture the value inside the pattern match.
Not a big change but would make it easier to read and express intent better IMO.

@cisen
Copy link
Contributor Author

cisen commented Jan 25, 2020

LGTM! Maybe a match statement would be a better fit instead of the if statements and would also allow you to recapture the value inside the pattern match.
Not a big change but would make it easier to read and express intent better IMO.

Thanks for your advice! I will improve it later. Because of Chinese New Year and my hometown's slow network,It will take a little time.

@IovoslavIovchev
Copy link
Contributor

Looks good but can you look for some commented tests for this or add some if they don't exist?

@cisen
Copy link
Contributor Author

cisen commented Jan 27, 2020

Looks good but can you look for some commented tests for this or add some if they don't exist?

Ok, I will add it later.

@cisen
Copy link
Contributor Author

cisen commented Jan 31, 2020

There is still some problems when I add tests like jerryscripts'. It will will take more times to fix this problems.

@cisen cisen closed this Mar 4, 2020
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.

3 participants