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

Truncate Typed Arrays (#441) #576

Merged
merged 7 commits into from
Jan 5, 2016

Conversation

lucasfcosta
Copy link
Member

Please don't merge yet, I'm still doing some experiments

As I said on the issue itself (#441), I'm gonna leave this PR here while working on this change so you guys can review my code and guarantee everything's fine.

Notes/Questions:

  1. It's not possible to check for a single TypedArray constructor, we have to check each one of them.
  2. Which TypedArrays should be considered "too long" and how are we going to truncate them?
  3. Shouldn't we truncate long Arrays too? If the answer for this is "yes" I feel like this code could be improved.

Progress:

  • Tests
  • isTypedArray()
  • formatTypedArray()
  • Truncating TypedArray
  • Refactor

@lucasfcosta
Copy link
Member Author

This is it for now.
The basics are done, please take a look at the notes I left on the PR's text.
I'm going to refactor it soon in order to get cleaner and more elegant code.

Edit:
I've just taken a look at the code and it seems alright. I have already fixed some code-style/grammar problems and everything seems to be in order. I'd be very grateful if any of you could review it and point me any other details I've forgotten or other things which should have been done.

objectToString(ar) === '[object Uint32Array]' ||
objectToString(ar) === '[object Float32Array]' ||
objectToString(ar) === '[object Float64Array]');
}
Copy link
Member

Choose a reason for hiding this comment

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

I suppose it is open to abuse, but you could just use:

/\w+Array]$/.test(objectToString(ar))

@keithamus
Copy link
Member

@lucasfcosta the general direction you're going with this is very good. Despite the two comments above, I'm happy with this code.

@lucasfcosta
Copy link
Member Author

Thank you!
I'm gonna take a look at your comments and make it as best as I can!

I'm a fairly new programmer, so your orientation helps me a lot.
Thank you again!

@lucasfcosta
Copy link
Member Author

Hi @keithamus, sorry for this time without commits (christmas and everything postponed my GitHub plans hahahaha)

Well, I've made the changes you pointed on this PR and tests are running smoothly. But as you've already questioned: what's the best way to show two in a comparison? I too am not aware of the most common TypedArray's use cases therefore I'm not sure what the best format would be.

About arrays: shouldn't they be truncated or not according to the config.truncateThreshold too?

@keithamus
Copy link
Member

@lucasfcosta thanks for sticking with this, I completely appreciate that you can't dedicate 100% of your time to Chai, and in fact I would certainly not want you to. Hope you enjoyed your christmas holidays 😄.

I don't think I was particularly clear about truncateThreshold, and that was my fault - something which I will correct now:

The way truncateThreshold works right now, is that if the Stringified value above a certain length, then instead of displaying the stringified value, we display a truncated value (see objDisplay.js). For example if you have [ 'hello world', 'goodbye world', 'something else' ] (stingified, this is over 40 characters) we'll instead display the truncated [ Array(3) ]. Importantly, truncateThreshold is configurable with a sensible default because you might think 40 chars is too short for your particular project, which is fine. Obviously this is a rather crude way to limit the amount of data displayed in an error message, but for the most part if kind of works.

For TypedArrays, where we know all values are ints of a fixed upper length (they're only numbers after all), we can reasonably predict the length of the string during parse time and therefore offer better truncation heuristics, so, using the truncateThreshold we can keep useful info about the array but also keep this threshold for reasonable output.

To demonstrate this with code, I'd expect some code a bit like this:

str = '[ ';
for (var i in output) {
  if (str.length >= config.truncateThreshold - 7) {
    str += '...';
    break;
  }
  str += output[i] + ', ';
}
str += ' ]';

The above code stringifies the array from within the loop, breaking as soon as the string is about to go over the threshold. Importantly: this doesn't rely on an arbitrary array length - but the configurable truncateTreshold property.

Hopefully this makes sense.

@lucasfcosta
Copy link
Member Author

Thanks, @keithamus, everything makes sense now!
Would you like to show TypedArrays the same way as Arrays, for example: [ Uint8Array(3) ] or would you like to use ... as I've thought before?

@keithamus
Copy link
Member

@lucasfcosta Nope, I think the way Arrays are currently displayed is not great. Ideally we'd have all objects represent themselves truncated by ellipsis and [ Array(n) ] style notation as a worst-case scenario. I'm going to put some time in soon to get all other values behaving exactly like this PR - but this is a fantastic start to where we want to go.

I'll merge this now as it's looking great.

keithamus added a commit that referenced this pull request Jan 5, 2016
@keithamus keithamus merged commit ec8d99f into chaijs:4.x.x Jan 5, 2016
@keithamus
Copy link
Member

@lucasfcosta if you feel like you need to put any more work into this - feel free to put another 4.x.x PR up. Personally I feel this is exactly where it should be though, so kudos 🎉!

@lucasfcosta
Copy link
Member Author

@keithamus Ok then, thanks for the help!

@lucasfcosta lucasfcosta changed the title [WIP] Truncate Typed Arrays (#441) Truncate Typed Arrays (#441) Jun 2, 2016
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