Summary: | check-webkit-style: Create a class to encapsulate reading text files | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Jerdonek <cjerdonek> | ||||||
Component: | Tools / Tests | Assignee: | 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
Chris Jerdonek
2010-04-17 09:59:51 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.)
Created attachment 53599 [details]
Proposed patch 2
Added docstrings to ProcessorBase and included more docstring information elsewhere about **kwargs.
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? (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. > 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?
(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. :) > 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.
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".) |