feat: `~/.config/autoimport/config.toml` support, `affect_indented_imports` configuration #172

HoverHell · 2021-12-09T16:27:11Z

Prior discussion: #169 (comment)

I opted to merge two configs instead of looking for them in order.

As always, if there are no objections to the general changes, I'll add tests and docs.

Checklist

Add test cases to all the changes you introduce
Update the documentation for the changes

…ports` configuration

coveralls · 2021-12-09T16:39:51Z

Pull Request Test Coverage Report for Build 1559663820

25 of 27 (92.59%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.3%) to 95.105%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/autoimport/services.py	19	21	90.48%

Totals
Change from base Build 1555497773:	-0.3%
Covered Lines:	272
Relevant Lines:	286

💛 - Coveralls

lyz-code · 2021-12-09T22:17:12Z

src/autoimport/services.py


 from autoimport.model import SourceCode

+# To consider: use `xdg.xdg_config_home() / "autoimport" / "config.toml"`


I feel that the merging of multiple files could be supported at maison level. Let me ask the developer if he'd like the feature, so we can save all this logic

You can follow up in this issue

@dbatten5 agreed on having the logic at maison level, do you mind migrating the code to a PR in their repo?

Hey @HoverHell , the feature is available on the new version of maison (1.3.0)!

lyz-code · 2021-12-09T22:21:08Z

tests/e2e/test_cli.py

@@ -10,6 +10,9 @@
 from autoimport.entrypoints.cli import cli
 from autoimport.version import __version__

+# When invoking the CLI for tests, ignore the config outside the repository.
+COMMON_OPTIONS = ["--no-global-config"]


Instead of creating the --no-global-config just for the tests (please correct me if there is any other use case of that flag), I think it would be better to set the environment variable pretty much like how pydantic config object works. Let me ask this too to the developer of maison

I just realized that this could be implemented at click level as they support loading values from environment variables So in the runner fixture we could set the value

lyz-code · 2021-12-09T22:22:37Z

src/autoimport/model.py

@@ -52,6 +52,8 @@ def __init__(
        self.code: List[str] = []
        self.config: Dict[str, Any] = config if config else {}
        self._trailing_newline = False
+        self._affect_indented_imports = self.config.get("affect_indented_imports", True)


In what case would you be interested in not changing the indented imports?

Answering your reply below:

There are many reasons to move all the imports to the top:

It's stated by PEP8

Imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants.

It's more efficient

If you want to load some modules only if a function is run, it's best to use lazy loading. Some links of information about it:

https://files.bemusement.org/talks/OSDC2008-FastPython/

https://stackoverflow.com/questions/4177735/best-practice-for-lazy-loading-python-modules

https://snarky.ca/lazy-importing-in-python-3-7/

https://levelup.gitconnected.com/python-trick-lazy-module-loading-df9b9dc111af

I planned to do an article in the blue book about it, but I haven't found the time yet

There are many reasons to move all the imports to the top

That's why it's the majority of cases; but if the import wasn't written on the top, perhaps there's a reason for it. Lazy loading is just one of those possible reasons. And even for that reason, as suggested in one of the links:

https://stackoverflow.com/questions/4177735/best-practice-for-lazy-loading-python-modules

simply importing a module within function is enough for it.

The only opposite use-case I can think of, is writing non-common imports near the code and get autoimport to move them to the top. Which is why it at least makes sense to make it configurable. Although that sounds more like an autoflake's job rather than autoimport.

But if you configure it in your pyproject.toml none of the indented imports of the project would be moved to the top. I'd rather use the # noqa: autoimport on those particular lines instead of having it as a configuration.

But maybe I'm being too obtuse on this, what do you think @Jasha10 @muriel?

I feel conflicted about this.
Here is my datapoint:

Before I started using autoimport, I would occasionally put imports inside of functions or in the middle of a module. My main reasons for wanting a given import to be near its call-site were enhanced readability and greater ease of refactoring.

A few examples of indented imports I've seen in the wild:

within an if TYPE_CHECKING block

branching based on python version e.g. importlib.resources for python3.7 vs importlib_resources for python3.6.

It's good to know that # noqa: autoimport is an option for granular control in these cases.

In my mind, this is a question of how opinionated we want autoimport to be -- should it follow the mold of black, which has very few configuration options? The other extreme would be to follow flake8, which allows granular configuration of individual behaviors on a per-scope basis.

Pros of the granular-control flake8 model:

gives users more latitude to selectively "break the rules"

May encourage user contribution of new config options (such as this PR)

Pros of the opinionated black model:

Using highly-opinionated tools can give a sense of security and predictability.

Easier to implement and maintain. This is especially true given that black (and autoimport) deal with source-code transformations, so enabling/disabling one behavior may have nontrivial interaction with other behaviors. Contrast this with flake8, where disabling one specific error should have no effect on whether other errors are caught.

Thank you for your comments @Jasha10, you helped me clear up my mind. I think autoimport should support the two cases you mentioned of indented imports, in fact, the if TYPE_CHECKING it's already supported. The other one is more difficult to implement so as for now we'll have to use the noqa comment.

Answering the question on how opinionated we want it to be, I'll go for the black path with the idea to maximize the maintainability. I'd agree to add user configurations when they raise them and they don't break good coding practices.

So I'd say that in this case, if you want to have indented imports that are not moved to the top, you should use the noqa comment, sorry @HoverHell :(

lyz-code · 2021-12-09T22:36:41Z

src/autoimport/services.py


 from autoimport.model import SourceCode

+# To consider: use `xdg.xdg_config_home() / "autoimport" / "config.toml"`
+GLOBAL_CONFIG_PATH = "~/.config/autoimport/config.toml"
+CONFIG_PROJECT_NAME = "autoimport"


I agree with you that using xdg.xdg_config_home() / "autoimport" / "config.toml" is a better solution than hardcoding ~/.config

HoverHell · 2021-12-10T10:13:53Z

In what case would you be interested in not changing the indented imports?

Right now `autoimport` moves the function-level imports to the module-level, which is not what I need. What was the initial intention of such behaviour?

Jasha10 · 2021-12-30T20:23:49Z

@HoverHell are you still interested in working on this?
There have been two updates to maison since this PR was opened:

If this PR is no longer on your radar, just say the word so that someone else can take over the diff :)

HoverHell · 2021-12-31T09:26:23Z

If this PR is no longer on your radar, just say the word so that someone else can take over the diff :)

I'm using these changes locally, and not sure if I will have the time to do further work towards merging within the next month.

…

Message ID: ***@***.***>

lyz-code · 2021-12-31T09:45:53Z

Thanks for the answer @HoverHell. The PR is open then for anyone that wants to adopt it and implement the changes suggested above.

If you want to finish it yourself, please say so in the comments to avoid duplication of work.

Thanks! :)

lyz-code · 2022-02-09T16:23:17Z

Closing the PR as it's been implemented by @Jasha10 on #206

Thank you for opening the path @HoverHell

feat: ~/.config/autoimport/config.toml support, `affect_indented_im…

54fb261

…ports` configuration

HoverHell mentioned this pull request Dec 9, 2021

Support a config file with example imports #169

Closed

lyz-code requested changes Dec 9, 2021

View reviewed changes

lyz-code mentioned this pull request Dec 9, 2021

Merge multiple config files dbatten5/maison#61

Closed

lyz-code reviewed Dec 9, 2021

View reviewed changes

lyz-code force-pushed the master branch from 67fe10c to 0ffa53d Compare February 3, 2022 16:32

Jasha10 mentioned this pull request Feb 6, 2022

Implement global config .config/autoimport/config.toml #206

Merged

2 tasks

lyz-code closed this Feb 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: `~/.config/autoimport/config.toml` support, `affect_indented_imports` configuration #172

feat: `~/.config/autoimport/config.toml` support, `affect_indented_imports` configuration #172

HoverHell commented Dec 9, 2021 •

edited

Loading

coveralls commented Dec 9, 2021

lyz-code Dec 9, 2021

lyz-code Dec 9, 2021

lyz-code Dec 10, 2021

lyz-code Dec 23, 2021

lyz-code Dec 9, 2021

lyz-code Dec 9, 2021

lyz-code Dec 9, 2021

lyz-code Dec 10, 2021

HoverHell Dec 11, 2021

lyz-code Dec 13, 2021

Jasha10 Dec 13, 2021 •

edited

Loading

lyz-code Dec 14, 2021

lyz-code Dec 9, 2021

HoverHell commented Dec 10, 2021 via email

Jasha10 commented Dec 30, 2021 •

edited

Loading

HoverHell commented Dec 31, 2021 via email

lyz-code commented Dec 31, 2021

lyz-code commented Feb 9, 2022


		from autoimport.model import SourceCode

		# To consider: use `xdg.xdg_config_home() / "autoimport" / "config.toml"`

feat: ~/.config/autoimport/config.toml support, affect_indented_imports configuration #172

feat: ~/.config/autoimport/config.toml support, affect_indented_imports configuration #172

Conversation

HoverHell commented Dec 9, 2021 • edited Loading

Checklist

coveralls commented Dec 9, 2021

Pull Request Test Coverage Report for Build 1559663820

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jasha10 Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HoverHell commented Dec 10, 2021 via email

Jasha10 commented Dec 30, 2021 • edited Loading

HoverHell commented Dec 31, 2021 via email

lyz-code commented Dec 31, 2021

lyz-code commented Feb 9, 2022

feat: `~/.config/autoimport/config.toml` support, `affect_indented_imports` configuration #172

feat: `~/.config/autoimport/config.toml` support, `affect_indented_imports` configuration #172

HoverHell commented Dec 9, 2021 •

edited

Loading

Jasha10 Dec 13, 2021 •

edited

Loading

Jasha10 commented Dec 30, 2021 •

edited

Loading