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

adjust representation of internal Items #36

Merged
merged 1 commit into from Sep 16, 2014
Merged

adjust representation of internal Items #36

merged 1 commit into from Sep 16, 2014

Conversation

seanmonstar
Copy link
Member

fixes #35

@reem I think this takes care of it.

@reem
Copy link
Contributor

reem commented Sep 15, 2014

Why do we keep the raw representation around? Since changes in the typed representation aren't reflected in the raw representation, I don't see why we should.

Either we should disallow parsing under two different types or we should come up with a scheme to propagate changes between the two.

@seanmonstar
Copy link
Member Author

I don't follow. The raw data is kept to parse a new header if the type is
different.

@reem
Copy link
Contributor

reem commented Sep 15, 2014

I can set the value of a header's typed representation, then parse it under a new representation and I won't get the updated value.

I think that would be a non-trivial source of bugs, and also makes libraries much more dependent on one another.

@seanmonstar
Copy link
Member Author

Oh I see. So, you think we should stick with the enum, and return None if
the typeids don't match?
On Sep 14, 2014 11:07 PM, "Jonathan Reem" notifications@github.com wrote:

I can set the value of a header's typed representation, then parse it
under a new representation and I won't get the updated value.


Reply to this email directly or view it on GitHub
#36 (comment).

@reem
Copy link
Contributor

reem commented Sep 15, 2014

I've got a branch somewhere with most of a representation that just converts to the raw representation if you ask it for a different type. I'll probably PR later tonight or tomorrow.

@seanmonstar
Copy link
Member Author

In that case, it's possible that a previous representation dropped some
data...
On Sep 14, 2014 11:23 PM, "Jonathan Reem" notifications@github.com wrote:

I've got a branch somewhere with most of a representation that just
converts to the raw representation if you ask it for a different type. I'll
probably PR later tonight or tomorrow.


Reply to this email directly or view it on GitHub
#36 (comment).

@reem
Copy link
Contributor

reem commented Sep 15, 2014

That is also possible. I think both representations have their pros and cons and I'm not sure which is best.

@reem
Copy link
Contributor

reem commented Sep 15, 2014

At minimum this can be refactored to not store the TypeId or the raw representation if we decide to not allow re-parsing.

seanmonstar added a commit that referenced this pull request Sep 16, 2014
adjust representation of internal Items
@seanmonstar seanmonstar merged commit b8e3178 into master Sep 16, 2014
@reem reem deleted the issue-35 branch September 27, 2014 22:43
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.

Using a different Header representation can lead to an illegal transmute
2 participants