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

Node create, append, before, and after #12

Merged
merged 7 commits into from
Oct 7, 2018

Conversation

edwardloveall
Copy link
Contributor

This adds for new methods to Node: create, append, before, and after. They enable documents to be modified to in addition to being parsed and searched.

I feel good about the append, before, and after methods, but I feel like create could be better. I'd love some feedback on that 🙂 One thing I tried was exposing the @raw_html instance variable from Parser, but that seemed like a strange thing to make public just so a different class could use it. As it stands, it feels like I'm too much in one method for a single class. Maybe the answer is just to abstract all the setup of creating and initializing a structure and tree into a different method so it doesn't look as cluttered. Ideally there's some other tree that's created already that I can use.

Also, I didn't create any documentation. When you think this looks okay, I'll happily add documentation to the methods before it's merged.

Thank you!

@kostya
Copy link
Owner

kostya commented Oct 7, 2018

Thanks. I think need to rename methods to append_child, insert_before, insert_after (this is clearer names). also move this methods to node/mutation.cr

about create, feels strange to create tree each time when node created. need to think more about it. is it really make sense to create independent trees?

may be we can do something like:

tree = Myhtml::Tree.new
div = tree.create_node(:div)
tree.insert_in_document(div)
a = tree.create(:a)
div.insert_after(a)

i think i refactor and split Parser and Tree classes.

@kostya
Copy link
Owner

kostya commented Oct 7, 2018

i split parser and tree in master: 55ed2ba

I went back and forth on how to name these. It's weird because OO
doesn't mix well with English. English uses Subject verb object. OO
uses Object.verb(subject).

So "Alice comes before Bob" is how it's said, but in code that's
`bob.before(alice)`. I also considered `bob.follow(alice)` but that
seemed too clever. Nokogiri uses before and after also.
@edwardloveall
Copy link
Contributor Author

Thanks for the comments and the quick refactor! I'll rename the insert methods and move create to the new Tree class. I'll post questions if I have them here.

It works the same but you need to create a tree first, then a node in
that tree. The node can then be inserted into a document.
@edwardloveall
Copy link
Contributor Author

Methods renamed/moved! I didn't add tree.insert_in_document because I wasn't sure what it would do. If you'd like that added I'm happy to do it, but I need some clarification on what it does.

it "adds a node just prior to this node" do
document = Myhtml::Parser.new("<html><body><main></main></body></html>")
main = document.css("main").first
tree = Myhtml::Tree.new
Copy link
Owner

Choose a reason for hiding this comment

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

you can use document.tree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Fixed.

@kostya
Copy link
Owner

kostya commented Oct 7, 2018

seems all ok, i add some changes after, anything you want to add here? docs?

@edwardloveall
Copy link
Contributor Author

Yes, I'll add docs and submit those before you merge. Thanks for all your help!

@edwardloveall
Copy link
Contributor Author

Docs added.

@kostya kostya merged commit de5dec2 into kostya:master Oct 7, 2018
@edwardloveall edwardloveall deleted the el-doc-modification branch October 7, 2018 17:43
@kostya
Copy link
Owner

kostya commented Oct 7, 2018

released v1.2.0 with this changes, example usage: https://github.com/kostya/myhtml/blob/master/examples/create_html.cr

@edwardloveall
Copy link
Contributor Author

Fantastic! 🎉 Thank you for such a quick turn around and a great library.

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.

2 participants