Azure / azure-sdk-for-python

This repository is for active development of the Azure SDK for Python. For consumers of the SDK we recommend visiting our public developer docs at https://learn.microsoft.com/python/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-python.
MIT License
4.63k stars 2.84k forks source link

Replace list() and dict() with literals #38583

Closed atombrella closed 20 hours ago

atombrella commented 5 days ago

Description

It's quite time-consuming to change instances of dict(a="hello", b="world") and I don't plan on doing that!

All SDK Contribution checklist:

General Guidelines and Best Practices

Testing Guidelines

github-actions[bot] commented 5 days ago

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

azure-sdk commented 5 days ago

API change check

API changes are not detected in this pull request.

atombrella commented 4 days ago

I don't really understand the test failures, and why this PR cause failing builds.

Note that there are tons of https://docs.python.org/3/library/datetime.html#datetime.datetime.utcnow in the code! They are also highlighted as warnings in the test runs.

iscai-msft commented 3 days ago

Thanks for opening this PR @atombrella!

Here is my analysis of the failing steps:

  1. The code changes have not been formatted (see failing ci here). Follow steps here for formatting
  2. I think the test failures might not be related to you. @annatisch it seems that cosmos is failing with service request errors, are you aware of any cosmos issue? Thanks!
atombrella commented 3 days ago

@iscai-msft Thank you!

  1. The code changes have not been formatted (see failing ci here). Follow steps here for formatting

My newest commit was done using this commit.

azure-sdk-for-python/scripts/devops_tasks on  feature/dict_list_literal [!] via azure-sdk 
➜ tox run -e black -c ../../eng/tox/tox.ini -- .

It changed lines I didn't touch in this PR. Maybe it'd be better to reformat and add the git-commit hash as described in https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view ? Actually, it seems a lot of the code base hasn't been reformatted according to black. At least black started formatting lots of files inside when I ran it inside the sdk-folder and then canceled it quickly.

iscai-msft commented 3 days ago

@atombrella i think some of the black formatting of files not originally touched by you have made their way into the PR, can you remove those? Thanks!

atombrella commented 2 days ago

@iscai-msft I deleted the commit. black then fails :/

I think the approach outlined in https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view is a good solution for the failing black/formatting tests. black formats boatloads of lines I didn't touch in my changes.

Note that snippets similar (and identical) to this is also in various places of the code-base.

        # Consider this is real string
        try:
            if isinstance(data, unicode):  # type: ignore
                return data
        except NameError:
            return str(data)
        else:
            return str(data)
iscai-msft commented 2 days ago

@atombrella are you able to run the black command from within each of the sdks you're touching with your changes, instead of over the entire repo? Thank you so much again for doing this!

atombrella commented 2 days ago

I am not a fan of this! git blame will be useless after this :(

iscai-msft commented 2 days ago

I understand your frustration, it does look like black has been behaving weird in our pipelines. @scbedd is actually working on this in a separate pr (linked here). He should be almost done with this PR, once this is in, do you mind pulling and trying again to see if the black issues are fixed?

Thank you so much again for your contribution, and apologies for it not going super smoothly, but your work is greatly appreciated.

iscai-msft commented 2 days ago

talked with @scbedd offline and we'll handle re-running this PR ourselves once his fix is in. Thanks for your patience

scbedd commented 2 days ago

/azp run python - pullrequest

azure-pipelines[bot] commented 2 days ago
Azure Pipelines successfully started running 1 pipeline(s).
scbedd commented 2 days ago

Ok. Analyze is now clean after my changes to black.

The test failures on azure-cosmos are known. @iscai-msft please double-check my verification before signoff, but if you only see setup failures invoking azure-cosmos as part of python - pullrequest, then IMO this PR is ready to go in.

iscai-msft commented 1 day ago

yep this looks good. Thanks so much @scbedd and @atombrella for doing this awesome work! I'm going to approve and @atombrella you should be able to merge when you're ready. Please ping if cosmos ends up blocking your ability to merge

Thanks so much again @atombrella for all of your work and patience

atombrella commented 1 day ago

@iscai-msft @scbedd I do not have merge rights to the Azure organization.

iscai-msft commented 20 hours ago

/check-enforcer validate

github-actions[bot] commented 20 hours ago

For help using check enforcer, see https://aka.ms/azsdk/checkenforcer

Available commands:

If you are initializing a new service, follow the new service docs. If no Azure Pipelines are desired, run /check-enforcer override.

iscai-msft commented 20 hours ago

/check-enforcer override

iscai-msft commented 20 hours ago

ok merged it, thank you so much again for your contribution and your patience @atombrella. Thanks for working with us, it's been a pleasure on our part