Skip to content

Conversation

@SamyPesse
Copy link

This PR adds a “offset” property (readable, not writable), to indicate the current offset in the buffer/stream.

I need this PR when using Dissolve to parse a git packfile, where I need to record a map {offset: Object} for future references.

@SamyPesse
Copy link
Author

@deoxxa ping 🔔

@arthurschreiber
Copy link
Collaborator

@SamyPesse Is there really a need for moveOffset here? Can't we just drop the local offset variable and use this.offset throughout?

@SamyPesse
Copy link
Author

@arthurschreiber Yes, we probably can, I'll run some tests and get back to you in a few minutes.

@SamyPesse
Copy link
Author

@arthurschreiber offset and this.offset are different:

  • offset is relative to the current _transform call
  • this.offset is relative to the whole buffer

During unit tests both are equal since we create a new instance for each write, but if you run example.js with a log:

moveOffset offset=1 that.offset=1
moveOffset offset=3 that.offset=3
moveOffset offset=5 that.offset=5
{ id: 1,
  payload: { asdf: { a: 2, b: 3, __proto__: null }, __proto__: null } }
moveOffset offset=1 that.offset=6
moveOffset offset=5 that.offset=10
moveOffset offset=9 that.offset=14
{ id: 2,
  payload: { asdf: { x: 4, y: 5, __proto__: null }, __proto__: null } }
moveOffset offset=1 that.offset=15
moveOffset offset=2 that.offset=17
moveOffset offset=2 that.offset=19
{ id: 1,
  payload: { asdf: { a: 2, b: 3, __proto__: null }, __proto__: null } }
moveOffset offset=1 that.offset=20
moveOffset offset=5 that.offset=24
moveOffset offset=13 that.offset=32
{ id: 3,
  payload:
   { asdf: { l: 2.0999999046325684, m: 2.1, __proto__: null },
     __proto__: null } }

I can try to write a cleaner solution, for example something where offset is replaced by (that.offset - baseOffset).

@articice
Copy link

articice commented Dec 6, 2016

@SamyPesse Hi! I have contributed one solution that doesn't use offset #24.

Personally, I think in many cases knowing the offset would force the user into "doing the math" by hand, which is usually not comfortable and messy.

But, for the sake of universality, yes, knowing the offset should be there for last resort.

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