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

Allow anchors redefinition/overwriting #222

Merged
merged 6 commits into from
Dec 19, 2016

Conversation

mlaily
Copy link
Contributor

@mlaily mlaily commented Nov 28, 2016

This pull request allows anchors to be redefined further down the stream instead of throwing a DuplicateAnchorException.

From the 1.1 spec as well as the 1.2 spec:

3.2.2.2. Anchors and Aliases

When composing a representation graph from serialized events, an alias node refers to the most recent node in the serialization having the specified anchor. Therefore, anchors need not be unique within a serialization.

This pull request also generates more human-friendly anchors (of the form "A", "B", ..., "AA", "AB"...) when saving a stream - see the last commit.

Don't be too afraid by the added lines count. Most of it is from yaml test data :)

@aaubry
Copy link
Owner

aaubry commented Dec 4, 2016

This looks good, except for the anchor names part. The name generation algorithm seems more complex than it needs to be, and I am not sure that generating anchors in caps is more human-friendly than numbers.
Maybe I could merge the anchor redefinition code and we can discuss the anchor names separately?

@mlaily
Copy link
Contributor Author

mlaily commented Dec 6, 2016

Fair enough. I removed the last commit.

The "human-friendly" side is more about generating sequential combinations of letters rather than random numbers.
And I agree the algorithm looks complex, but I'm not sure it's avoidable if we also want it to be efficient.

@aaubry aaubry merged commit 01ff789 into aaubry:master Dec 19, 2016
@aaubry
Copy link
Owner

aaubry commented Dec 19, 2016

Thanks.

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.

None yet

2 participants