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

DataFactory methods to create from existing term and quad | closes #137 #158

Merged
merged 3 commits into from
Apr 15, 2019

Conversation

elf-pavlik
Copy link
Member

As discussed in #137

@elf-pavlik
Copy link
Member Author

👋

Copy link
Contributor

@blake-regalia blake-regalia left a comment

Choose a reason for hiding this comment

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

Would be great to support plain objects as well and make these methods more robust, e.g., not requiring original to have a .equals method.

@blake-regalia
Copy link
Contributor

I'm pretty sure that with #142 we could simply say, such that newObject.equals(original) as this would cover all cases including plain objects, correct?

@elf-pavlik
Copy link
Member Author

Can we do it the other way and require that newly created instance will have .equals() method and passing original to it returns true ? Since it looks like spec will include definition of .equals() #150 I think that fromTerm and fromQuad definitions should simply take advantage of already eisting definition of .equals()

@blake-regalia
Copy link
Contributor

@elf-pavlik I think that is exactly what I am proposing... do we have a misunderstanding?

@elf-pavlik
Copy link
Member Author

Thanks for commit @blake-regalia 👍
No misunderstanding, just race condition on github comments 🐎

fixed markup
@elf-pavlik
Copy link
Member Author

@dmitrizagidulin I noticed you had some suggestions in #81 (comment) would you like to review this PR?

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