Bug 37754

Summary: check-webkit-style: Create a class to encapsulate reading text files
Product: WebKit Reporter: Chris Jerdonek <cjerdonek>
Component: Tools / TestsAssignee: Chris Jerdonek <cjerdonek>
Status: RESOLVED FIXED    
Severity: Normal CC: cjerdonek, hamaji
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 37066    
Attachments:
Description Flags
Proposed patch
none
Proposed patch 2 hamaji: review+

Description Chris Jerdonek 2010-04-17 09:59:51 PDT
This is a step towards separating our style-checking code from the code that iterates over paths.
Comment 1 Chris Jerdonek 2010-04-17 10:55:26 PDT
Created attachment 53597 [details]
Proposed patch

Rather than changing the working code incrementally, I decided to add the class we'll be switching to as a whole, with more or less full unit test coverage.  I believe this will make the subsequent steps smaller and a bit easier to understand -- since we'll just need to start using a class that already exists and is fully tested.  (I'm not sure the existing code actually tests reading files anyways.  I think it tests with mock methods in all cases.)
Comment 2 Chris Jerdonek 2010-04-17 11:21:54 PDT
Created attachment 53599 [details]
Proposed patch 2

Added docstrings to ProcessorBase and included more docstring information elsewhere about **kwargs.
Comment 3 Shinichiro Hamaji 2010-04-18 18:26:41 PDT
Comment on attachment 53599 [details]
Proposed patch 2

Looks good! Sorry for the latency. A few comments:

How about changing all single quotes to double quotes at this chance?

> +    class TestProcessor(ProcessorBase):

How about naming this as MockProcessor?
Comment 4 Chris Jerdonek 2010-04-18 19:09:37 PDT
(In reply to comment #3)
> (From update of attachment 53599 [details])
> Looks good! Sorry for the latency. A few comments:

Thanks!  No problem -- especially since it was the weekend.

> How about changing all single quotes to double quotes at this chance?

I was actually making an effort to use single quotes. :)  I don't think we ever decided this as a group.  Personally, I started out preferring double quotes but then changed to preferring single quotes for the reason I mentioned here:

http://trac.webkit.org/wiki/PythonGuidelines

Also see--

https://bugs.webkit.org/show_bug.cgi?id=37062

Feel free to edit the wiki page, by the way, if you disagree or have additional thoughts.

> > +    class TestProcessor(ProcessorBase):
> 
> How about naming this as MockProcessor?

Sounds good.
Comment 5 Shinichiro Hamaji 2010-04-18 19:43:57 PDT
> I was actually making an effort to use single quotes. :)  I don't think we ever
> decided this as a group.  Personally, I started out preferring double quotes
> but then changed to preferring single quotes for the reason I mentioned here:

Ah, I see. I didn't notice you changed your mind :) So, let me comment again - how about changing double quotes to single quotes?
Comment 6 Chris Jerdonek 2010-04-18 20:09:17 PDT
(In reply to comment #5)
> > I was actually making an effort to use single quotes. :)  I don't think we ever
> > decided this as a group.  Personally, I started out preferring double quotes
> > but then changed to preferring single quotes for the reason I mentioned here:
> 
> Ah, I see. I didn't notice you changed your mind :) So, let me comment again -
> how about changing double quotes to single quotes?

Did you see any spots in my patch that I missed aside from this?

> +        dir = os.path.join(self._temp_dir, "foo_dir")

I'm starting to realize there are many distinct cases where the choice between double/single needs to be made -- not just when enclosing string literals, but also inside code comments and log messages, for example.  For those I don't pretend to have a rule yet. :)
Comment 7 Shinichiro Hamaji 2010-04-18 20:16:24 PDT
> Did you see any spots in my patch that I missed aside from this?
> 
> > +        dir = os.path.join(self._temp_dir, "foo_dir")

Yeah, I agree this is the only literal which uses double-quotes.
Comment 8 Chris Jerdonek 2010-04-18 21:30:40 PDT
Committed: http://trac.webkit.org/changeset/57803

(I will change the code in the above patch to match the logic discussed here:

https://bugs.webkit.org/show_bug.cgi?id=37122#c21

in the patch to make this patch "live".)