Write and test BlockNode, ListNode #73

sydneyli · 2019-10-04T02:26:27Z

From interface described in #49, depends on #72. Should be rebased/merged when #72 lands.

codecov-io · 2019-10-04T02:31:37Z

Codecov Report

Merging #73 into master will increase coverage by 3.19%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
+ Coverage   88.34%   91.54%   +3.19%     
==========================================
  Files           7        6       -1     
  Lines         678      627      -51     
==========================================
- Hits          599      574      -25     
+ Misses         79       53      -26

Impacted Files	Coverage Δ
apacheconfig/__init__.py	`100% <ø> (ø)`	⬆️
apacheconfig/parser.py	`85.45% <0%> (-0.52%)`	⬇️
apacheconfig/lexer.py	`96.65% <0%> (+0.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5741248...b7baae8. Read the comment docs.

etingof

Thank you! Looks good, my main concerns are:

Do we really need ContentsNode abstraction?
I guess I am not yet understanding specialized parser suite creation frequency - it would be great to have it reusable as much as possible

WDYT?

apacheconfig/wloader.py

etingof · 2019-10-30T19:49:10Z

apacheconfig/wloader.py

+        """
+
+
+class ContentsNode(Node):


How much ContentsNode is different from BlockNode? I can imagine, structurally, having BlockNode everywhere (root, intermediate nodes) and ItemNode & BlockNode in it.

My understanding is that with ContentsNode we will have them scoped like:

ContentsNode (root) -> BlockNode -> ContentsNode -> BlockNode -> ItemNode

So I am wondering if we can always treat BlockNode as a "container", where ItemNode would always be a leaf scalar e.g.:

BlockNode (root) -> BlockNode -> ItemNode

WDYT?

apacheconfig/wloader.py

etingof · 2019-11-05T05:45:56Z

@sydneyli I assume this PR is not blocked on me, presently. ;-)

sydneyli · 2019-11-06T18:25:07Z

@sydneyli I assume this PR is not blocked on me, presently. ;-)

Yep, it's back to me! Hoping to get to this today.

sydneyli · 2019-11-07T00:20:53Z

Re: ContentsNode vs BlockNode:

It's a good point. I do see what you mean that ContentsNode vs BlockNode are really only unique in that contents has no tag, and effectively acts as a root node (or as a child of BlockNode), and BlockNode is intermediary.

Here are the things that make the two different:

BlockNode isolates the logic for manipulating & accessing the tag name & values.
BlockNode keeps track of the whitespace preceding the first tag. ContentsNode keeps track of the whitespace after the final scalar in the group (like, the whitespace before EOF, or before the closing tag of a block)

Having these (particularly the tag manipulation) separated by class type seems useful. What if we have BlockNode subclass a generic ContentsNode, and maybe rename them something like MidNode and RootNode, respectively? Something that'd come up is that the whitespace property of the classes would conflict, so ContentsNode would have to keep a unique trailing_whitespace property.

etingof · 2019-11-07T08:56:28Z

Having these (particularly the tag manipulation) separated by class type seems useful. What if we have BlockNode subclass a generic ContentsNode, and maybe rename them something like MidNode and RootNode, respectively? Something that'd come up is that the whitespace property of the classes would conflict, so ContentsNode would have to keep a unique trailing_whitespace property.

That sounds like a legit design to me. Alternatively to subclassing, may be we could have an abstract or a base class declaring the interfaces and/or common implementation, then RootNode and BlockNode would have their own unique stuff.

In other words:

AbstractNode -> CommonNodeMixIn + RootNode
AbstractNode -> CommonNodeMixIn + BlockNode

or

NodeBase -> RootNode
NodeBase -> BlockNode

The mix-in piece would be there to keep abstract class defining just the interfaces, not any common code. On the other hand, that's not strictly required with Python customs, AFAIK.

WDYT?

sydneyli · 2019-11-07T19:31:10Z

Alternatively to subclassing, may be we could have an abstract or a base class declaring the interfaces and/or common implementation, then RootNode and BlockNode would have their own unique stuff.

Subclassing might make things a bit simpler here, since BlockNode has a lot of unique functionality over RootNode, but not the other way around. That is, the set of things RootNode should be able to do is actually a subset of the things BlockNode can do.

EDIT: This was a pretty straightforward change, so I tried it out in the most recent commit. Let me know what you think, or if you might prefer having an abstract class. Also, naming is still hard, so will take recommendations there :)

Also remove exported *Node objects from __init__ since we don't necessarily want to expose those constructors.

etingof

Looks awesome to me!

I have a handful of nits for your considerations.

apacheconfig/wloader.py

etingof · 2019-11-09T10:17:21Z

apacheconfig/wloader.py

+
+    @property
+    def arguments(self):
+        """Returns arguments for this block as a literal string.


literal string - is it in Apache config syntax?

Nope-- removed to just say "string".

apacheconfig/wloader.py

sydneyli · 2019-11-13T01:09:37Z

Thanks for the review! I did a pass on the documentation. ~~Currently working on error handling documentation & tests, as well testing for unicode support.~~

EDIT: Did a pass on error handling. Unicode support might be a larger undertaking, so I opened #85 !

sydneyli · 2019-11-22T23:50:36Z

(Just a note in case you missed-- this and the unicode PR are good for another round when you get the chance!)

etingof · 2019-11-24T18:20:58Z

(Just a note in case you missed-- this and the unicode PR are good for another round when you get the chance!)

Hey, and thank you for the ping!

I am not always sure at which point it's back to me (I am checking out the PRs from occasionally), so explicit communication is good!

etingof · 2019-11-24T18:27:47Z

apacheconfig/wloader.py

+    Every AST should have a :class:`apacheconfig.ListNode` at its root. A
+    :class:`apacheconfig.ListNode` or :class:`apacheconfig.BlockNode` can have
+    other :class:`apacheconfig.BlockNode`s and
+    :class:`apacheconfig.LeafASTNode`s as children.


nit: Is it deliberate to have AST in abstract and leaf, but not in other class names?

etingof · 2019-11-24T18:30:30Z

apacheconfig/wloader.py

-    root should be a ListNode. To construct from a raw string, use the
-    `parse` factory function. The default __init__ constructor expects data
-    from the internal apacheconfig parser.
+    Every configuration file's root should be a ListNode.


nit: hyperlink ListNode?

etingof · 2019-11-24T18:36:17Z

Let's merge this PR - I think it's good enough and I do not want to block you.

I've left a couple of nits for your consideration (I am never short of nits). If you consider them worth addressing, I guess you could just push a quick PR.

Also, perhaps at some point we should have a CHANGELOG entry explaining this undertaking. I am just not sure at which PR in the chain it's would be best to note.

Thank you!

sydneyli · 2019-11-29T18:10:50Z

Thanks @etingof -- appreciate it! I agree with your notes on consistency between the namings, so i'll push a PR for that soon.

Create and test Node and ItemNode

9e8eb4f

sydneyli mentioned this pull request Oct 4, 2019

Test writable loader with apache config files #74

Draft

sydneyli added 10 commits October 4, 2019 16:03

Address some nits.

7f3e021

change ordering of imports

6b7b4be

Fix flake8 and Sphinx-ify docs

321af49

Move factory function to ItemNode.parse

62e29ab

Clean up docstrings and abstract methods

8879312

napoleon-ize the docs

4eaade0

Create and test BlockNode and ContentsNode

addd125

lint

092bb36

merge with upstream API changes

eb01706

napoleonize and cleanup the docs

9aed274

sydneyli force-pushed the writable-loader-contents branch from db18f4a to 9aed274 Compare October 8, 2019 00:40

etingof reviewed Oct 30, 2019

View reviewed changes

sydneyli added 2 commits November 6, 2019 14:24

Merge branch 'master' into writable-loader-contents

b60f523

Address initial comments from etingof

3d11a53

sydneyli added 2 commits November 7, 2019 11:51

BlockNode subclasses ContentsNode, rename ContentsNode to ListNode

5074b02

Also remove exported *Node objects from __init__ since we don't necessarily want to expose those constructors.

Linting and fixing up comments

ccc367a

etingof reviewed Nov 9, 2019

View reviewed changes

etingof changed the title ~~Write and test BlockNode, ContentsNode~~ Write and test BlockNode, ListNode Nov 9, 2019

Comb through and fix documentation nits.

c2f04dc

Comb through and clean up error handling.

b7baae8

etingof reviewed Nov 24, 2019

View reviewed changes

etingof merged commit 896349e into etingof:master Nov 24, 2019

sydneyli deleted the writable-loader-contents branch November 25, 2019 16:07

sydneyli mentioned this pull request Nov 29, 2019

chores: writable loader naming, docs, and changelog #88

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Write and test BlockNode, ListNode #73

Write and test BlockNode, ListNode #73

sydneyli commented Oct 4, 2019

codecov-io commented Oct 4, 2019 •

edited

Loading

etingof left a comment

etingof Oct 30, 2019

etingof commented Nov 5, 2019

sydneyli commented Nov 6, 2019

sydneyli commented Nov 7, 2019 •

edited

Loading

etingof commented Nov 7, 2019 •

edited

Loading

sydneyli commented Nov 7, 2019 •

edited

Loading

etingof left a comment

etingof Nov 9, 2019

sydneyli Nov 13, 2019

sydneyli commented Nov 13, 2019 •

edited

Loading

sydneyli commented Nov 22, 2019

etingof commented Nov 24, 2019

etingof Nov 24, 2019

etingof Nov 24, 2019

etingof commented Nov 24, 2019

sydneyli commented Nov 29, 2019

Write and test BlockNode, ListNode #73

Write and test BlockNode, ListNode #73

Conversation

sydneyli commented Oct 4, 2019

codecov-io commented Oct 4, 2019 • edited Loading

Codecov Report

etingof left a comment

Choose a reason for hiding this comment

etingof Oct 30, 2019

Choose a reason for hiding this comment

etingof commented Nov 5, 2019

sydneyli commented Nov 6, 2019

sydneyli commented Nov 7, 2019 • edited Loading

etingof commented Nov 7, 2019 • edited Loading

sydneyli commented Nov 7, 2019 • edited Loading

etingof left a comment

Choose a reason for hiding this comment

etingof Nov 9, 2019

Choose a reason for hiding this comment

sydneyli Nov 13, 2019

Choose a reason for hiding this comment

sydneyli commented Nov 13, 2019 • edited Loading

sydneyli commented Nov 22, 2019

etingof commented Nov 24, 2019

etingof Nov 24, 2019

Choose a reason for hiding this comment

etingof Nov 24, 2019

Choose a reason for hiding this comment

etingof commented Nov 24, 2019

sydneyli commented Nov 29, 2019

codecov-io commented Oct 4, 2019 •

edited

Loading

sydneyli commented Nov 7, 2019 •

edited

Loading

etingof commented Nov 7, 2019 •

edited

Loading

sydneyli commented Nov 7, 2019 •

edited

Loading

sydneyli commented Nov 13, 2019 •

edited

Loading