Closed Bug 1424148 Opened 6 years ago Closed 6 years ago

Local duplicate of global constant created in function in "parse_histograms.py"

Categories

(Toolkit :: Telemetry, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: adbugger, Assigned: adbugger, Mentored)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171129230835

Steps to reproduce:

In parse_histograms.py, in the 'strict_type_checks' clause of the check_name function, the local variable 'pattern' is defined, which is same as the regex defined globally in 'CPP_IDENTIFIER_PATTERN'. This was likely missed in a previous refactor. Replacing the pattern usage with "CPP_IDENTIFIER_PATTERN" seems to be the correct fix.
I worked on some changes to parse_histograms.py for bug 1401612, when I found this duplication. Is it ok to ask to be assigned to this whenever it is triaged (assuming the status is changed to NEW)?
Sure, i assigned it to you!
Assignee: nobody → adibhar97
Mentor: chutten
Priority: -- → P3
Alright, I think I need a little help with versions on hg. So I made some changes to the parse_histograms.py as part of bug 1401612. And now the same file must be changed. Here is the diff of the changes:

--- a/toolkit/components/telemetry/parse_histograms.py
+++ b/toolkit/components/telemetry/parse_histograms.py
@@ -291,10 +291,9 @@ associated with the histogram.  Returns 
         # the histogram names to a strict pattern.
         # We skip this on the server to avoid failures with old Histogram.json revisions.
         if self._strict_type_checks:
-            pattern = '^[a-z][a-z0-9_]+[a-z0-9]$'
-            if not re.match(pattern, name, re.IGNORECASE):
+            if not re.match(CPP_IDENTIFIER_PATTERN, name, re.IGNORECASE):
                 ParserError('Error for histogram name "%s": name does not conform to "%s"' %
-                            (name, pattern)).handle_later()
+                            (name, CPP_IDENTIFIER_PATTERN)).handle_later()
 
     def check_expiration(self, name, definition):
         field = 'expires_in_version'

The thing is, I wrote the ParserError().handle_later() syntax as a fix for 1401612, pending review. So do I wait for that patch to be checked in, or is there something I can do to revert to a previous code state and then make changes? In that case, won't the patch for bug 1401612 lead to merge conflicts?

I guess what I'm asking is how do you work on multiple bugs at once and ensure no conflicts if multiple bugs require changes to the same file? As of now, I'm just waiting for the other patch for 1401612 to be checked in and become part of the code base.
Flags: needinfo?(chutten)
Given how close bug 140612 is to completion, you've done the right thing writing this solution on top of that solution. Things get a little more hectic if we were landing these at about the same time and needed to make sure one landed first (there are bugzilla keywords for it), but I think we'll be able to finish bug 140612 before this one so it shouldn't come to that :)
Flags: needinfo?(chutten)
(In reply to Chris H-C :chutten from comment #4)
> Given how close bug 140612 is to completion, you've done the right thing
> writing this solution on top of that solution. Things get a little more
> hectic if we were landing these at about the same time and needed to make
> sure one landed first (there are bugzilla keywords for it), but I think
> we'll be able to finish bug 140612 before this one so it shouldn't come to
> that :)

By finish, you meant check-in or VERIFIED/RESOLVED (bug 1401612)?

Also, should I wait for a status change to NEW before working on this bug?

In any case, I'll submit a patch soon and then let you decide when to review/check it in.
Attached patch 1424148_v1.patchSplinter Review
Uploading patch according to diff made in comment 3, review subject to conditions in comment 5.
Attachment #8937661 - Flags: review?(chutten)
By "finish" I meant it landing in the same integration tree that this one would land in. (Or waiting for it to land in mozilla-central if we wanted to play it safe) :)

I have marked the bug as ASSIGNED which is something we sometimes forget to do, opting to assign it to someone to show that they're working on it even if the bug is in UNCONFIRMED or NEW.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8937661 - Flags: review?(chutten) → review+
Well, that was quick. One of these days you should find yourself a complicated one :)

Need help finding something?
Keywords: checkin-needed
Sure, I'd love some help finding something more complicated. :D
What did you have in mind?
It depends where your interests and skills are. Ping me on IRC (https://wiki.mozilla.org/Irc) in channel #telemetry and we can more easily chat. Or, if that's a pain, here's a small scattering of resources:

A list of "good second bugs": https://mzl.la/2CFygIS
The entire list of everything open within Toolkit::Telemetry: https://bugzilla.mozilla.org/buglist.cgi?product=Toolkit&component=Telemetry&resolution=---&list_id=13938275
The Bugs Ahoy bug finder: https://www.joshmatthews.net/bugsahoy/
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9744e7abcf9
Local duplicate of global constant used in function. r=chutten
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f9744e7abcf9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: