-
Notifications
You must be signed in to change notification settings - Fork 4
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
adding sssom support #109
adding sssom support #109
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just lurking feel free to ignore comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does the config live that says: "use narrow matches for node replacement" vs "us only exact matches for node replacement"?
I... don't know if we have one, to be honest |
Oh, so in response to the config question- I think for now we might just bring this in with only "exact match", and open it up to include narrow in another PR |
Restricting to exact sounds good as a first safe step, but you may loose quite a bit of relevant Information. I think it's worth making that configurable via some property like "merge_on_property". |
nice one @hrshdhgd thanks for taking a look at that!! |
is there anything preventing this from being merged as is? |
There's a lot of formatting noise in this now, but the relevant changes are in:
|
addresses #108
fix #110
Tasks:
Clarify existing examples (this can/should probably be a separate issue)see Clarify examples #113