Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

{Pylint} Fix use-dict-literal #30308

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from
Open

Conversation

atombrella
Copy link
Contributor

Related command

Various commands are "affected" by this. Please check the diff.

Description

flake8-comprehensions has the two suggestions I try to remedy.

    Rewrite set([]) as set()
    Rewrite dict([]) as {}

You can check the marginal benefits with timeit in ipython.

I wonder why these two rules are disabled in pylintrc. Maybe it's for historic reasons.

    use-dict-literal,
    consider-using-dict-items,

Copy link

azure-client-tools-bot-prd bot commented Nov 11, 2024

️✔️AzureCLI-FullTest
️✔️acr
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️acs
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️advisor
️✔️latest
️✔️3.12
️✔️3.9
️✔️ams
️✔️latest
️✔️3.12
️✔️3.9
️✔️apim
️✔️latest
️✔️3.12
️✔️3.9
️✔️appconfig
️✔️latest
️✔️3.12
️✔️3.9
️✔️appservice
️✔️latest
️✔️3.12
️✔️3.9
️✔️aro
️✔️latest
️✔️3.12
️✔️3.9
️✔️backup
️✔️latest
️✔️3.12
️✔️3.9
️✔️batch
️✔️latest
️✔️3.12
️✔️3.9
️✔️batchai
️✔️latest
️✔️3.12
️✔️3.9
️✔️billing
️✔️latest
️✔️3.12
️✔️3.9
️✔️botservice
️✔️latest
️✔️3.12
️✔️3.9
️✔️cdn
️✔️latest
️✔️3.12
️✔️3.9
️✔️cloud
️✔️latest
️✔️3.12
️✔️3.9
️✔️cognitiveservices
️✔️latest
️✔️3.12
️✔️3.9
️✔️compute_recommender
️✔️latest
️✔️3.12
️✔️3.9
️✔️computefleet
️✔️latest
️✔️3.12
️✔️3.9
️✔️config
️✔️latest
️✔️3.12
️✔️3.9
️✔️configure
️✔️latest
️✔️3.12
️✔️3.9
️✔️consumption
️✔️latest
️✔️3.12
️✔️3.9
️✔️container
️✔️latest
️✔️3.12
️✔️3.9
️✔️containerapp
️✔️latest
️✔️3.12
️✔️3.9
️✔️core
️✔️2018-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️cosmosdb
️✔️latest
️✔️3.12
️✔️3.9
️✔️databoxedge
️✔️2019-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️dls
️✔️latest
️✔️3.12
️✔️3.9
️✔️dms
️✔️latest
️✔️3.12
️✔️3.9
️✔️eventgrid
️✔️latest
️✔️3.12
️✔️3.9
️✔️eventhubs
️✔️latest
️✔️3.12
️✔️3.9
️✔️feedback
️✔️latest
️✔️3.12
️✔️3.9
️✔️find
️✔️latest
️✔️3.12
️✔️3.9
️✔️hdinsight
️✔️latest
️✔️3.12
️✔️3.9
️✔️identity
️✔️latest
️✔️3.12
️✔️3.9
️✔️iot
️✔️2019-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️keyvault
️✔️2018-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️lab
️✔️latest
️✔️3.12
️✔️3.9
️✔️managedservices
️✔️latest
️✔️3.12
️✔️3.9
️✔️maps
️✔️latest
️✔️3.12
️✔️3.9
️✔️marketplaceordering
️✔️latest
️✔️3.12
️✔️3.9
️✔️monitor
️✔️latest
️✔️3.12
️✔️3.9
️✔️mysql
️✔️latest
️✔️3.12
️✔️3.9
️✔️netappfiles
️✔️latest
️✔️3.12
️✔️3.9
️✔️network
️✔️2018-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️policyinsights
️✔️latest
️✔️3.12
️✔️3.9
️✔️privatedns
️✔️latest
️✔️3.12
️✔️3.9
️✔️profile
️✔️latest
️✔️3.12
️✔️3.9
️✔️rdbms
️✔️latest
️✔️3.12
️✔️3.9
️✔️redis
️✔️latest
️✔️3.12
️✔️3.9
️✔️relay
️✔️latest
️✔️3.12
️✔️3.9
️✔️resource
️✔️2018-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️role
️✔️latest
️✔️3.12
️✔️3.9
️✔️search
️✔️latest
️✔️3.12
️✔️3.9
️✔️security
️✔️latest
️✔️3.12
️✔️3.9
️✔️servicebus
️✔️latest
️✔️3.12
️✔️3.9
️✔️serviceconnector
️✔️latest
️✔️3.12
️✔️3.9
️✔️servicefabric
️✔️latest
️✔️3.12
️✔️3.9
️✔️signalr
️✔️latest
️✔️3.12
️✔️3.9
️✔️sql
️✔️latest
️✔️3.12
️✔️3.9
️✔️sqlvm
️✔️latest
️✔️3.12
️✔️3.9
️✔️storage
️✔️2018-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️synapse
️✔️latest
️✔️3.12
️✔️3.9
️✔️telemetry
️✔️2018-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️util
️✔️latest
️✔️3.12
️✔️3.9
️✔️vm
️✔️2018-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9

Copy link

Hi @atombrella,
Since the current milestone time is less than 7 days, this pr will be reviewed in the next milestone.

Copy link

azure-client-tools-bot-prd bot commented Nov 11, 2024

️✔️AzureCLI-BreakingChangeTest
️✔️Non Breaking Changes

@yonzhan
Copy link
Collaborator

yonzhan commented Nov 11, 2024

Thank you for your contribution! We will review the pull request and get back to you soon.

@microsoft-github-policy-service microsoft-github-policy-service bot added the customer-reported Issues that are reported by GitHub users external to the Azure organization. label Nov 11, 2024
@kairu-ms kairu-ms assigned bebound and jiasli and unassigned kairu-ms Nov 12, 2024
@bebound
Copy link
Contributor

bebound commented Nov 12, 2024

Thanks for your contribution.

The disabled rules were introduced in #20192. I think it's okay to fix them. Could you please also remove them from pylintrc?

@bebound bebound changed the title {Misc} Replace dict with a literal. set([]) with set() {Misc} Replace dict() with {}, set([]) with set() Nov 12, 2024
@atombrella
Copy link
Contributor Author

Thanks for your contribution.

The disabled rules were introduced in #20192. I think it's okay to fix them. Could you please also remove them from pylintrc?

I can do that, but it'll be quite a bit of work. You unfortunately still accept PRs with dict().

@jiasli
Copy link
Member

jiasli commented Nov 13, 2024

use-dict-literal: https://pylint.readthedocs.io/en/latest/user_guide/messages/refactor/use-dict-literal.html
consider-using-dict-items: https://pylint.pycqa.org/en/latest/user_guide/messages/convention/consider-using-dict-items.html

The disabled rules were introduced in #20192.

#20192 was created by me. I ignored them simply because we didn't have bandwidth to work on those issues.

So yes, please also remove them from

use-dict-literal,

@jiasli
Copy link
Member

jiasli commented Nov 14, 2024

I will fix consider-using-dict-items in #30341.

@jiasli jiasli mentioned this pull request Nov 14, 2024
12 tasks
@jiasli
Copy link
Member

jiasli commented Nov 15, 2024

@atombrella, could you remove

use-dict-literal,

@jiasli jiasli changed the title {Misc} Replace dict() with {}, set([]) with set() {Pylint} Fix use-dict-literal Nov 20, 2024
kairu-ms
kairu-ms previously approved these changes Nov 20, 2024
@atombrella
Copy link
Contributor Author

I've fixed them. There weren't that many after all :) There are quite of cases like:

        if self.__loadbalancer_models is None:
            load_balancer_models = {}
            load_balancer_models["ManagedClusterLoadBalancerProfile"] = self.ManagedClusterLoadBalancerProfile
            load_balancer_models[
                "ManagedClusterLoadBalancerProfileManagedOutboundIPs"
            ] = self.ManagedClusterLoadBalancerProfileManagedOutboundIPs
            load_balancer_models[
                "ManagedClusterLoadBalancerProfileOutboundIPs"
            ] = self.ManagedClusterLoadBalancerProfileOutboundIPs
            load_balancer_models[
                "ManagedClusterLoadBalancerProfileOutboundIPPrefixes"
            ] = self.ManagedClusterLoadBalancerProfileOutboundIPPrefixes
            load_balancer_models["ResourceReference"] = self.ResourceReference
            self.__loadbalancer_models = SimpleNamespace(**load_balancer_models)
        return self.__loadbalancer_models

If you use PyCharm, it provides a quick and easy way to directly create the dictionary in a single-step.

https://pypi.org/project/flake8-comprehensions/ has lots of valuable suggestions. I've fixed the dict.fromkeys in a couple of cases, as well as the list comprehensions.

@bebound
Copy link
Contributor

bebound commented Nov 21, 2024

More errors are discovered after disabling use-dict-literal:

Some use-dict-literal issue can be fixed with ruff check ./src --select C408 --unsafe-fixes, but C408 may also fix other linter issue.

Ref: astral-sh/ruff#970 (comment)

@atombrella
Copy link
Contributor Author

More errors are discovered after disabling use-dict-literal:

Some use-dict-literal issue can be fixed with ruff check ./src --select C408 --unsafe-fixes, but C408 may also fix other linter issue.

Ref: astral-sh/ruff#970 (comment)

I did check with ruff to find all of them :) I hadn't heard of ruff before, it seems to quite useful/powerful. C408 is indeed a superset of https://pylint.readthedocs.io/en/stable/user_guide/messages/refactor/use-dict-literal.html

There are many great suggestions from flake8-comprehensions. For instance, I experimented briefly today with one of the examples from https://www.geeksforgeeks.org/python-dictionary-fromkeys-method/ to check the performance improvements of using dict.fromkeys instead of {x: None for x in iterable} It's about ~15%. I fixed the single case that this flake8 plugin found, as well as a few of the list comprehensions.

Note that in some places, I reformatted the code slightly. IMO it improves readability to split comprehensions over more lines. One case is:

info_dict = {
    lun: {"managedDisk": {'storageAccountType': None}}
    for lun in dummy_expected if lun != "os"
}

@atombrella
Copy link
Contributor Author

@bebound Could you help with the failed "Azure.azure-cli (Codegen Coverage)". I don't think my changes are related.

@wangzelin007
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@wangzelin007
Copy link
Member

wangzelin007 commented Nov 25, 2024

I created a PR, but I couldn't reproduce the issue. I'll try running /azp run to see if it resolves the issue.

@atombrella
Copy link
Contributor Author

atombrella commented Nov 25, 2024

I created a PR, but I couldn't reproduce the issue. I'll try running /azp run to see if it resolves the issue.

Seems to be good again :) Everything is green.

@kairu-ms @bebound @jiasli

@@ -107,11 +107,14 @@ def test_subscription_recording_processor_for_response(self):

for template in body_templates:
mock_sub_id = str(uuid.uuid4())
mock_response = dict({'body': {}})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This dict() is a no-op, not sure why it was written like that in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-reported Issues that are reported by GitHub users external to the Azure organization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants