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

RESOLVED FIXED in Firefox 55

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Dexter, Assigned: pgadige__, Mentored)

Tracking

Trunk
mozilla55
Points:
1
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
+++ 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
(Reporter)

Updated

2 years ago
No longer depends on: 1344855
(Assignee)

Comment 1

2 years ago
I'd like to work on this issue. please assign it to me. Thank you.
(Reporter)

Comment 2

2 years ago
(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
(Reporter)

Comment 3

2 years ago
The dependency just landed. You can start working on this bug if you want!
(Assignee)

Comment 4

2 years ago
Made the fix on my local machine. Need guidance on submitting the patch for review.

Comment 5

2 years ago
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 :)
(Assignee)

Comment 6

2 years ago
Made the necessary fix. Removed has_key() method. Instead used `in` operator. Attached patch file.
Attachment #8845663 - Flags: review+
Attachment #8845663 - Flags: feedback+

Comment 7

2 years ago
@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.
(Assignee)

Comment 8

2 years ago
(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
(Assignee)

Updated

2 years ago
Attachment #8845663 - Flags: review?(alessio.placitelli)
Attachment #8845663 - Flags: review+
Attachment #8845663 - Flags: feedback?(alessio.placitelli)
(Reporter)

Updated

2 years ago
Attachment #8845663 - Flags: feedback?(alessio.placitelli)
Attachment #8845663 - Flags: feedback+
(Reporter)

Comment 9

2 years ago
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.
(Reporter)

Comment 12

2 years ago
(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)
Comment hidden (mozreview-request)
Did you get a chance to review the code patch,Alessio?
(Reporter)

Comment 16

2 years ago
(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.
(Reporter)

Comment 17

2 years ago
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)
(Reporter)

Comment 18

2 years ago
mozreview-review
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 hidden (mozreview-request)
(Reporter)

Comment 20

2 years ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 22

2 years ago
mozreview-review-reply
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.
(Reporter)

Comment 23

2 years ago
mozreview-review
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+

Comment 24

2 years ago
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)
(Reporter)

Comment 25

2 years ago
(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)
(Assignee)

Comment 26

2 years ago
mozreview-review-reply
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)
(Reporter)

Comment 28

2 years ago
(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)
(Reporter)

Comment 29

2 years ago
Pooja, are you still interested in working on this?
Flags: needinfo?(poojagadige)
Comment hidden (mozreview-request)
(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.

Comment 32

2 years ago
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

Comment 33

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ee2cfbed2e29
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Reporter)

Updated

2 years ago
Flags: needinfo?(poojagadige)
status-firefox54: affected → ---
You need to log in before you can comment on or make changes to this bug.