Bug 37754 – check-webkit-style: Create a class to encapsulate reading text files

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Jerdonek]

This is a step towards separating our style-checking code from the code that iterates over paths.

[Chris Jerdonek]

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.)

[Chris Jerdonek]

Created attachment 53599 [details]
Proposed patch 2

Added docstrings to ProcessorBase and included more docstring information elsewhere about **kwargs.

[Shinichiro Hamaji]

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?

[Chris Jerdonek]

(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.

[Shinichiro Hamaji]

> 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?

[Chris Jerdonek]

(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. :)

[Shinichiro Hamaji]

> 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.

[Chris Jerdonek]

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".)