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

Convert YamlNode to AbstractYamlNode #93

Closed
wants to merge 1 commit into from
Closed

Convert YamlNode to AbstractYamlNode #93

wants to merge 1 commit into from

Conversation

SherifWaly
Copy link
Contributor

PR for #91

I found that there are YamlMapping and AbstractYamlMapping, same for sequence.
So I removed YamlMapping and merged it with AbstractYamlMapping.

@coveralls
Copy link

coveralls commented Mar 13, 2017

Coverage Status

Coverage increased (+0.02%) to 86.529% when pulling e5e5601 on SherifWaly:#91 into 142ef59 on decorators-squad:master.

@SherifWaly
Copy link
Contributor Author

@amihaiemil Please check whether these changes are good or not. The problem came from the abstract classes as in PR description. Also I think some docs should be modified.

@amihaiemil
Copy link
Member

amihaiemil commented Mar 14, 2017

@SherifWaly The AbstractYamlNode looks good, but you have to revert the removal of YamlMapping and YamlSequence, because:

  • YamlMapping and YamlSequence are the interfaces that the user will work with -- see the repo's README.md for examples of how the user will work with the library.

  • On the other hand AbstractYamlMapping and AbstractYamlSequence are supposed to be super-classes for any concrete implementation of YamlMapping and YamlSequence: they provide generic methods such as equals, hashcode and compareTo which are supposed to work with any implementations of mapping and sequence.

Notice that the 2 interfaces are public, the user will work with them, while the abstract classes are not public, the user will never know they exist (they are what's generically called "details of implementation")

You might not see the big picture entirely because I did a lot of the architecture by myself -- I'll write an architecture.md file, where I'll describe everything in detail.

In the meantime (and I wanted to tell you this before, too), don't hesitate to open issues about anything that seems unclear to you :D

@SherifWaly
Copy link
Contributor Author

@amihaiemil Thanks for reviewing, I think YamlMapping and YamlSequence must be YamlNode so how to convert them to AbstractYamlNode and at the same time they are interfaces not classes?

@amihaiemil
Copy link
Member

@SherifWaly Yes indeed.. and there's also that Yaml dump part... I guess it's a little more complicated than I initially thought. I'll look into it as well and let you know. For now, don't do anything else :D

@amihaiemil
Copy link
Member

@SherifWaly I think it's better to leave this for now. It's quite a big change, as it seems, and I want to release the first version soon :)

@amihaiemil
Copy link
Member

@SherifWaly I'm closing this PR - the issue is postponed, and by the time we'll take care of it, this PR will long be outdated :D

@amihaiemil amihaiemil closed this Mar 21, 2017
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