Create and test Node and ItemNode #72

sydneyli · 2019-10-04T02:19:43Z

Fleshing out API as described in #49. Starting with generic Node and a more specific ItemNode implementation.

codecov-io · 2019-10-04T02:21:55Z

Codecov Report

❗ No coverage uploaded for pull request base (master@27845ec). Click here to learn what that means.
The diff coverage is 46.66%.

@@            Coverage Diff            @@
##             master      #72   +/-   ##
=========================================
  Coverage          ?   87.64%           
=========================================
  Files             ?        7           
  Lines             ?      688           
  Branches          ?        0           
=========================================
  Hits              ?      603           
  Misses            ?       85           
  Partials          ?        0

Impacted Files	Coverage Δ
apacheconfig/__init__.py	`100% <100%> (ø)`
apacheconfig/wloader.py	`43.85% <43.85%> (ø)`

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 27845ec...1fa014d. Read the comment docs.

etingof

Great start! I have some nits in-line.

apacheconfig/__init__.py

apacheconfig/wloader.py

sydneyli · 2019-10-04T23:04:58Z

Thanks for the feedback! I addressed some of the comments-- I'll try to address the rest of them with the following:

Sphinx-ify the docs
Flake8 on test_wloader.py

sydneyli · 2019-10-05T03:43:09Z

Documentation/testing sprint done! While doing that I also changed the constructor a bit-- I moved the parse_item factory as a static method at ItemNode.parse.

I pushed a branch at sydneyli/apacheconfig:merged-writable-loader-item-sphinx that merges this branch with #77 to show that the docs make the apacheconfig.wloader automodule properly.

etingof

I have some more nitpicks for your consideration. I hope you do not mind (yet). ;-)

apacheconfig/wloader.py

sydneyli · 2019-10-15T22:44:16Z

@etingof this is ready for more review, when you have time ^_^ Biggest change is moving the docs to Napoleon per your suggestion-- the docs are definitely a lot cleaner now.

etingof · 2019-10-16T06:25:17Z

@etingof this is ready for more review, when you have time ^_^ Biggest change is moving the docs to Napoleon per your suggestion-- the docs are definitely a lot cleaner now.

Awesome! Thank you for the heads up - I've been sidetracked by the upcoming conference. I will get back to this shortly.

etingof

I am sorry for it has taken that long!

I've done another round, I think the code is in a good shape! I have a couple of nits for your consideration.

apacheconfig/wloader.py

etingof

First of all, I am terribly sorry for long delays! But the good news is that I am hopefully back from my conference preparations!

I've posted some more nits, these are the side-effect of my trying to understand the new design. Those are mostly nits, in my mind they might be helpful for the human reader to understand the arrangements. WDYT?

I will now proceed to the next PRs.

apacheconfig/wloader.py

etingof · 2019-10-30T18:14:07Z

apacheconfig/wloader.py

+                       [_restore_original(word) for word in self._raw])))
+
+
+def native_apache_parser(options=None):


It seems that this method mainly applies some specific options to lexer/parser/config objects. In some sense, the instantiation code is not relevant to this purpose, right?

So what if we introduce some sort of syntax "flavors" i.e. sets of options, perhaps as global constants, that the user can just pick or mix and match to specialize parsing suite to the desired syntax?

Yes, I like the idea of having and exporting a set of common option configurations. Something like NATIVE_APACHE_PARSER_OPTIONS?

The motivation behind this was primarily to have a simpler interface for creating a native Apache parser-- would you prefer not to increase the surface area of exported functions to maintain? Either way is fine by me.

Yes, I like the idea of having and exporting a set of common option configurations. Something like NATIVE_APACHE_PARSER_OPTIONS?

Yeah... BTW, with current design the options are not only for the parser, but for lexer and loader as well.

The motivation behind this was primarily to have a simpler interface for creating a native Apache parser-- would you prefer not to increase the surface area of exported functions to maintain? Either way is fine by me.

I'd try not to introduce something overly specific and not likely reusable whenever possible.

How about this:

from apacheconfig import flavors from apachaconfig import make_loader with make_loader(**flavors.NATIVE_APACHE) as loader: ...

If we also need just parser or just lexer, would it make sense to have similar functions for them? Or may be we can rename & parameterize make_loader so that it would become a universal factory function?

Either way, this factory function(s) would be driven by pre-defined and/or user-specified options (flavors, in my snippet).

I'm a fan of exposing the option sets (flavors, as you put it) for the time being-- I have to think a bit more on what the factory function might look like, maybe once the other interfaces are in.

I am thinking, that functionally we specialize parser to work on a specific kind (flavor) of grammar. Each flavor can be composed from a bunch of options.

I gave it a shot with a flavors.py-- lmk what you think!

sydneyli · 2019-10-31T19:30:57Z

First of all, I am terribly sorry for long delays! But the good news is that I am hopefully back from my conference preparations!

No worries at all! Thanks for working with us on this :)

I've posted some more nits, these are the side-effect of my trying to understand the new design. Those are mostly nits, in my mind they might be helpful for the human reader to understand the arrangements. WDYT?

Naming is difficult, so I appreciate your input here! I think they make sense. Took your suggestions.

etingof

The new code makes sense to me so far!

I am curious if this factory context manager:

@contextmanager
def make_loader(**options):
    ApacheConfigLexer = make_lexer(**options)
    ApacheConfigParser = make_parser(**options)

    yield ApacheConfigLoader(ApacheConfigParser(ApacheConfigLexer()),
                             **options)

Could be generalized to simplify this code:

        ApacheConfigLexer = make_lexer(**flavors.NATIVE_APACHE)
         ApacheConfigParser = make_parser(**flavors.NATIVE_APACHE)
         self.parser = ApacheConfigParser(ApacheConfigLexer(), start="contents")

apacheconfig/flavors.py

sydneyli · 2019-11-01T21:58:59Z

Re: the make_loader context manager, could create a generic parser context manager, though probably name it something else (since make_parser already exists!) Either way, we could also punt this conversation to another issue for what sort of factory interface to provide for the writable loader.

etingof · 2019-11-01T22:01:22Z

Re: the make_loader context manager, could create a generic parser context manager, though probably name it something else (since make_parser already exists!) Either way, we could also punt this conversation to another issue for what sort of factory interface to provide for the writable loader.

Alright, so what's remaining for this PR to be merged? My only question so far is possible repetitive parser re-creation. Is this a valid concern?

sydneyli · 2019-11-01T22:03:20Z

Alright, so what's remaining for this PR to be merged? My only question so far is possible repetitive parser re-creation. Is this a valid concern?

That fix should be coming in the downstream PR!

sydneyli · 2019-11-01T22:04:00Z

(To be specific, since the LeafNode doesn't contain any other config items, it doesn't need a reference to a parser since it doesn't do any parsing)

etingof · 2019-11-01T22:11:27Z

Thank you! \o/

etingof · 2019-11-01T22:11:50Z

We should probably update CHANGES at some point.

etingof · 2019-11-01T22:15:45Z

Another unrelated thought: would it make sense to add "default" flavor essentially enumerating all options and their defaults? That won't technically change anything, but would probably be more illustrative and easier to change defaults.

sydneyli · 2019-11-01T22:19:01Z

I think that would be good! a lot of the defaults are scattered in the codebase (using dict.get(key, default)) so something like that would consolidate them in one place. I'll open an issue.

Create and test Node and ItemNode

9e8eb4f

sydneyli force-pushed the writable-loader-item branch from d0a6722 to 9e8eb4f Compare October 4, 2019 02:21

sydneyli mentioned this pull request Oct 4, 2019

Write and test BlockNode, ListNode #73

Merged

etingof reviewed Oct 4, 2019

View reviewed changes

Address some nits.

7f3e021

sydneyli added 2 commits October 4, 2019 16:06

change ordering of imports

6b7b4be

Fix flake8 and Sphinx-ify docs

321af49

Move factory function to ItemNode.parse

62e29ab

etingof reviewed Oct 5, 2019

View reviewed changes

apacheconfig/wloader.py Outdated Show resolved Hide resolved

apacheconfig/wloader.py Outdated Show resolved Hide resolved

apacheconfig/wloader.py Outdated Show resolved Hide resolved

apacheconfig/wloader.py Outdated Show resolved Hide resolved

apacheconfig/wloader.py Show resolved Hide resolved

etingof reviewed Oct 6, 2019

View reviewed changes

apacheconfig/wloader.py Outdated Show resolved Hide resolved

apacheconfig/wloader.py Outdated Show resolved Hide resolved

apacheconfig/wloader.py Outdated Show resolved Hide resolved

apacheconfig/wloader.py Outdated Show resolved Hide resolved

sydneyli added 2 commits October 7, 2019 15:47

Clean up docstrings and abstract methods

8879312

napoleon-ize the docs

4eaade0

etingof reviewed Oct 23, 2019

View reviewed changes

etingof reviewed Oct 24, 2019

View reviewed changes

apacheconfig/wloader.py Outdated Show resolved Hide resolved

Address comments and require parser

4a3843e

etingof reviewed Oct 30, 2019

View reviewed changes

Naming suggestions

c338d72

Expose set of options rather than explicit apache parser constructor

1fa014d

etingof reviewed Nov 1, 2019

View reviewed changes

apacheconfig/flavors.py Show resolved Hide resolved

etingof merged commit 5741248 into etingof:master Nov 1, 2019

sydneyli deleted the writable-loader-item branch November 1, 2019 22:12

sydneyli mentioned this pull request Nov 1, 2019

Consolidate default options in one place #82

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create and test Node and ItemNode #72

Create and test Node and ItemNode #72

sydneyli commented Oct 4, 2019

codecov-io commented Oct 4, 2019 •

edited

Loading

etingof left a comment

sydneyli commented Oct 4, 2019 •

edited

Loading

sydneyli commented Oct 5, 2019

etingof left a comment

sydneyli commented Oct 15, 2019

etingof commented Oct 16, 2019

etingof left a comment

etingof left a comment

etingof Oct 30, 2019 •

edited

Loading

sydneyli Oct 31, 2019

etingof Oct 31, 2019 •

edited

Loading

sydneyli Nov 1, 2019

etingof Nov 1, 2019

sydneyli Nov 1, 2019

sydneyli commented Oct 31, 2019

etingof left a comment

sydneyli commented Nov 1, 2019

etingof commented Nov 1, 2019

sydneyli commented Nov 1, 2019

sydneyli commented Nov 1, 2019

etingof commented Nov 1, 2019

etingof commented Nov 1, 2019

etingof commented Nov 1, 2019

sydneyli commented Nov 1, 2019

		[_restore_original(word) for word in self._raw])))


		def native_apache_parser(options=None):

Create and test Node and ItemNode #72

Create and test Node and ItemNode #72

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

sydneyli commented Oct 4, 2019 • edited Loading

sydneyli commented Oct 5, 2019

etingof left a comment

Choose a reason for hiding this comment

sydneyli commented Oct 15, 2019

etingof commented Oct 16, 2019

etingof left a comment

Choose a reason for hiding this comment

etingof left a comment

Choose a reason for hiding this comment

etingof Oct 30, 2019 • edited Loading

Choose a reason for hiding this comment

sydneyli Oct 31, 2019

Choose a reason for hiding this comment

etingof Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

sydneyli Nov 1, 2019

Choose a reason for hiding this comment

etingof Nov 1, 2019

Choose a reason for hiding this comment

sydneyli Nov 1, 2019

Choose a reason for hiding this comment

sydneyli commented Oct 31, 2019

etingof left a comment

Choose a reason for hiding this comment

sydneyli commented Nov 1, 2019

etingof commented Nov 1, 2019

sydneyli commented Nov 1, 2019

sydneyli commented Nov 1, 2019

etingof commented Nov 1, 2019

etingof commented Nov 1, 2019

etingof commented Nov 1, 2019

sydneyli commented Nov 1, 2019

codecov-io commented Oct 4, 2019 •

edited

Loading

sydneyli commented Oct 4, 2019 •

edited

Loading

etingof Oct 30, 2019 •

edited

Loading

etingof Oct 31, 2019 •

edited

Loading