On Thu, Mar 13, 2008 at 6:38 PM, Danek Duvall <[EMAIL PROTECTED]> wrote:
> On Wed, Mar 12, 2008 at 11:13:21PM -0500, Shawn Walker wrote:
>
> > http://cr.opensolaris.org/~swalker/pkg-244/
>
> filter.py:
>
> - I'm a bit confused by want_attr and want_value: are they ever anything
> other than opposites? You always seem to set them in concert.
Yeah, I did it more for my sanity than anything else.
When I was working on some of the weirder cases, it seemed like every
time I made a change it went "wonky" :-)
You're right though; I don't really need want_value. I've dropped it.
> - line 57,59: could NAME ever follow NAME or NUMBER follow NUMBER in a
> token stream? It seems like that would represent a broken filter if it
> could.
It's somewhat misleading as the order doesn't matter. next_tok here
only implies what the expected subsequent tokens can be, not what
order they have to be in. If you look at my tests, you'll see I have
cases where NAME follows NUMBER and vice versa.
> - line 62: should we assert "not want_attr"?
Unnecessary as next_tok prevents "" from happening. So we already get
an indirect assert for that case.
I don't need to check want_attr here though, but I do need to see if
we actually have anything to append.
> - line 64-66: presumably there won't be anything further after this
> token, right? So do you need these lines?
Good catch; I was too paranoid here.
> - line 70: extra comma
Yeah, sorry. I got confused about the tuple syntax again. For some
reason I thought I always needed a trailing one, but now I see I only
need a trailing comma if it's a single item tuple.
> - line 79: need space after comma.
Fixed.
> - line 82: maybe it's just me, but you might want to put a comment before
> this to explain why the tokens you're testing for before this are
> different from the tokens you're testing after. I think it might also
You are right; I should have had a lot more comments in general. I
have seasoned the code appropriately :-)
> be the case that want_value is always True here (and want_attr is
> False), so you might be able to do without the if entirely. But I'd
> still comment on the split you've made here.
Actually, want_attr can be true here. For example, the following filter:
(arch=386 & debug=false) | doc=true
want_attr would initially be true, and we would encounter a "(" token first.
Notably, we would never expect a "(" token to terminate the parsing of a value.
For example, this would never be valid:
attr=value (
We would always expect an ENDMARKER, ")", "&", or "|" token after a value.
There are a few other cases as well. So, I've heavily commented this
section and split up the logic so that it should be much clearer now.
> I'm assuming that the bulk of your changes in t_filter.py are tabs to
> spaces, plus the addition of the extra test cases, so I'm going to assume
> that's all correct. :)
Yes. I fixed pylint's complaints about the spacing, etc. and added
lots of tests (about 10?).
Make test passes, so I assume what I have is correct (well, that and I
manually checked the results of each test case to see that they looked
right to me).
I have updated the review materials (only filter.py changed):
webrev:
http://cr.opensolaris.org/~swalker/pkg-244/
hg bundle:
http://cr.opensolaris.org/~swalker/pkg-244/raw_files/pkg-244.hg
hg export:
http://cr.opensolaris.org/~swalker/pkg-244/raw_files/pkg-244.patch
--
Shawn Walker, Software and Systems Analyst
http://binarycrusader.blogspot.com/
"To err is human -- and to blame it on a computer is even more so." -
Robert Orben
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
