Closed Bug 1344858 Opened 7 years ago Closed 7 years ago

Enable flake8 rule W601: ".has_key() is deprecated, use 'in'"

Categories

(Toolkit :: Telemetry, enhancement, P3)

Product:
Toolkit
Component:
Telemetry
Type:
enhancement
Points:
1

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: Dexter, Assigned: pgadige__, Mentored)

References

Details

(Whiteboard: [measurement:client][lang=python])

Attachments

(1 file, 2 obsolete files)

Bug 1344858 - Enable flake8 rule W601: ".has_key() is deprecated, use 'in'".
59 bytes, text/x-review-board-request
Dexter
: review+
Details 
+++ This bug was initially created as a clone of Bug #1344855 +++

In bug 1332651 we landed flake8 initial support to lint Python files in the Telemetry directory, by disabling all the detected problems.

This bug is about enabling the W601, ".has_key() is deprecated, use 'in'":

1) Remove the W601 rule from the .flake8 file in https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry

2) Run "./mach lint -l flake8 toolkit/components/telemetry".

3) Fix the reported problems
No longer depends on: 1344855
I'd like to work on this issue. please assign it to me. Thank you.
(In reply to Pooja Gadige (:pgadige) from comment #1)
> I'd like to work on this issue. please assign it to me. Thank you.

Done, but please keep in mind that you won't be able to work on this until bug 1332651 lands.
Assignee: nobody → poojagadige
The dependency just landed. You can start working on this bug if you want!
Made the fix on my local machine. Need guidance on submitting the patch for review.
Hi Pooja, Use Need info tag so that the person who you are seeking information from is notified about your question.

If you have completed making the changes then you can apply hg commit to commit your changes to your repo. hg commit will bring up your edittor where you have to write a commit message:
Bug <Bug_number> - what you have done. r?<name_of_reviewer>

short description

after committing the changes you can use hg export tip > /path/to/some/file.patch

Thus your patch will be created ( a well formatted diff file)

Now you can upload that patch here as an attachment and set the review flag and chose a reviewer from the list.

Hope this helps :)
Made the necessary fix. Removed has_key() method. Instead used `in` operator. Attached patch file.
Attachment #8845663 - Flags: review+
Attachment #8845663 - Flags: feedback+
@Pooja

You will have to set the review flag to '?'. It means you are asking for review from a reviewer. You can select a suggested reviewer from the list. The reviewer after reviewing your patch will give you review+ or he will ask you to make more changes.
(In reply to Deepjyoti Mondal from comment #7)
> @Pooja
> 
> You will have to set the review flag to '?'. It means you are asking for
> review from a reviewer. You can select a suggested reviewer from the list.
> The reviewer after reviewing your patch will give you review+ or he will ask
> you to make more changes.

Thank you for the correction @Deepyoti
Attachment #8845663 - Flags: review?(alessio.placitelli)
Attachment #8845663 - Flags: review+
Attachment #8845663 - Flags: feedback?(alessio.placitelli)
Attachment #8845663 - Flags: feedback?(alessio.placitelli)
Attachment #8845663 - Flags: feedback+
Comment on attachment 8845663 [details] [diff] [review]
replaced has_key() with `in` operator

Pooja, I think you've attached the wrong patch: this is changing the blocklist.xml file.

Also, it looks like you're blocking incoming needinfo requests. Please check your Bugzilla settings.
Attachment #8845663 - Flags: review?(alessio.placitelli)
Alessio, after I executed `hg export . > my-change.patch` in the directory `mozilla-central`, I attached the generated file `my-change.patch` (an XML file).

Perhaps, I am missing a few intermediate steps. Though I made single change to the file 
`mozilla-central/toolkit/components/telemetry/histogram_tools.py` that held the deprecated `has_key()` method.

I'd appreciate if you could please give me an example of the file to be attached so that I know how one appears.

I unchecked the `blocking incoming needinfo` setting. 

Thank you.
Aha perhaps I must execute the command `hg export . > my-change.patch` inside the directory that holds `histogram_tools.py` - the file I made changes to. It could generate the correct `my-change.patch` file.
(In reply to Pooja Gadige (:pgadige) from comment #10)
> Alessio, after I executed `hg export . > my-change.patch` in the directory
> `mozilla-central`, I attached the generated file `my-change.patch` (an XML
> file).
> 
> Perhaps, I am missing a few intermediate steps. Though I made single change
> to the file 
> `mozilla-central/toolkit/components/telemetry/histogram_tools.py` that held
> the deprecated `has_key()` method.
> 
> I'd appreciate if you could please give me an example of the file to be
> attached so that I know how one appears.
> 
> I unchecked the `blocking incoming needinfo` setting. 
> 
> Thank you.

The format of the patch you've attached is correct, but you're not exporting your changes. You're probably just exporting the latest changeset in your local history.

As mentioned by @Deepjyoti in comment 5, you should be committing your changes before using the export command.

If you have troubles with this workflow, you could try using the new (and suggested) workflow using MozReview: see https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html
Uploaded correct .patch file reflecting desired changes - replaced deprecated .has_key() with `in` operator.
Attachment #8845663 - Attachment is obsolete: true
Attachment #8846156 - Flags: review?(alessio.placitelli)
Did you get a chance to review the code patch,Alessio?
(In reply to Pooja Gadige (:pgadige) from comment #15)
> Did you get a chance to review the code patch,Alessio?

No, not yet. Please be advised that reviews can take up to 48 hours on non-working days.
Comment on attachment 8846156 [details] [diff] [review]
replaced has_key() with `in` operator

I'm marking this as obsolete as the most recent version of the code is on MozReview.
Attachment #8846156 - Attachment is obsolete: true
Attachment #8846156 - Flags: review?(alessio.placitelli)
Comment on attachment 8846178 [details]
Bug 1344858 - Enable flake8 rule W601: ".has_key() is deprecated, use 'in'".

https://reviewboard.mozilla.org/r/119274/#review121616

This looks good overall, but please change the commit message as suggested below.

::: commit-message-34585:1
(Diff revision 1)
> +Bug 1344858 - removed W601 rule. replaced deprecated .has_key() with 'in' operator. r?Dexter

Please change the commit message to:

Bug 1344858 - Enable flake8 rule W601: ".has_key() is deprecated, use 'in'". r?dexter
Comment on attachment 8846178 [details]
Bug 1344858 - Enable flake8 rule W601: ".has_key() is deprecated, use 'in'".

https://reviewboard.mozilla.org/r/119274/#review122434

::: toolkit/components/telemetry/.flake8:3
(Diff revision 2)
>  [flake8]
>  # See http://pep8.readthedocs.io/en/latest/intro.html#configuration
> -ignore = E121, E123, E126, E129, E133, E226, E241, E242, E704, W503, E402, E302, E703, E502, E128, E501, E713, E231, E111, E261, E222, E203, E201, E202, W602, E127, F841, F401, W601
> +ignore = E121, E123, E126, E129, E133, E226, E241, E242, E704, W503, E402, E302, E703, E502, E128, E501, E713, E231, E111, E261, E222, E203, E201, E202, W602, E127, F401

Why are you removing rule F841?
Attachment #8846178 - Flags: review?(alessio.placitelli)
Comment on attachment 8846178 [details]
Bug 1344858 - Enable flake8 rule W601: ".has_key() is deprecated, use 'in'".

https://reviewboard.mozilla.org/r/119274/#review121616

> Please change the commit message to:
> 
> Bug 1344858 - Enable flake8 rule W601: ".has_key() is deprecated, use 'in'". r?dexter

Made necessary changes to the commit message.
Comment on attachment 8846178 [details]
Bug 1344858 - Enable flake8 rule W601: ".has_key() is deprecated, use 'in'".

https://reviewboard.mozilla.org/r/119274/#review123734

This looks good now, thank you!
Attachment #8846178 - Flags: review?(alessio.placitelli) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 5f095eae80f6 -d f34476fa83fa: rebasing 382772:5f095eae80f6 "Bug 1344858 - Enable flake8 rule W601: ".has_key() is deprecated, use 'in'". r=Dexter" (tip)
merging toolkit/components/telemetry/.flake8
merging toolkit/components/telemetry/histogram_tools.py
warning: conflicts while merging toolkit/components/telemetry/.flake8! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
(In reply to Mozilla Autoland from comment #24)
> We're sorry, Autoland could not rebase your commits for you automatically.
> Please manually rebase your commits and try again.
> 
> hg error in cmd: hg rebase -s 5f095eae80f6 -d f34476fa83fa: rebasing
> 382772:5f095eae80f6 "Bug 1344858 - Enable flake8 rule W601: ".has_key() is
> deprecated, use 'in'". r=Dexter" (tip)
> merging toolkit/components/telemetry/.flake8
> merging toolkit/components/telemetry/histogram_tools.py
> warning: conflicts while merging toolkit/components/telemetry/.flake8!
> (edit, then use 'hg resolve --mark')
> unresolved conflicts (see hg resolve, then hg rebase --continue)

Pooja, would you please rebase your patch and push the rebased one for review?
Flags: needinfo?(poojagadige)
Comment on attachment 8846178 [details]
Bug 1344858 - Enable flake8 rule W601: ".has_key() is deprecated, use 'in'".

https://reviewboard.mozilla.org/r/119274/#review122434

> Why are you removing rule F841?

I realised I haven't synced the updated version of the file before pushing the new review request.
Alessio, could you please guide me in rebasing the patch? On my system, I see changeset: 348025:50cb38cbbe47 as the tip on /mozilla-central repository

Thanks.
Flags: needinfo?(poojagadige) → needinfo?(alessio.placitelli)
(In reply to Pooja Gadige (:pgadige) from comment #27)
> Alessio, could you please guide me in rebasing the patch? On my system, I
> see changeset: 348025:50cb38cbbe47 as the tip on /mozilla-central repository
> 
> Thanks.

Please refer to this [1] guide for the suggested Firefox workflow.

In order to rebase, since you're using MozReview, do the following:

- Move to the bookmark for this commit (hg up BOOKMARKNAME).
- Assuming you're using firefoxtree, do hg pull
- Then hg rebase -d central

[1] - https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/firefoxworkflow.html
Flags: needinfo?(alessio.placitelli)
Pooja, are you still interested in working on this?
Flags: needinfo?(poojagadige)
(In reply to Alessio Placitelli [:Dexter] from comment #29)
> Pooja, are you still interested in working on this?

submitted new changeset number, after reverting previous changeset number changes.
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ee2cfbed2e29
Enable flake8 rule W601: ".has_key() is deprecated, use 'in'". r=Dexter
https://hg.mozilla.org/mozilla-central/rev/ee2cfbed2e29
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: needinfo?(poojagadige)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: