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

Describe isort's multi_line_output setting #109

Merged

Conversation

Jamim
Copy link
Contributor

@Jamim Jamim commented Aug 27, 2019

Hello,

These changes follow up #95.

The multi_line_output was already set to 3 in the Add initial black formatting PR, so after rebasing to master this commit contains only comment that describes a magic number from the isort configuration file.

Related discussions:

@Oberon00
Copy link
Member

Oberon00 commented Aug 27, 2019

Related: #36, #95 (comment).

@@ -64,8 +64,7 @@
from contextlib import contextmanager
import typing

from opentelemetry import loader
from opentelemetry import types
from opentelemetry import loader, types
Copy link
Member

@Oberon00 Oberon00 Aug 28, 2019

Choose a reason for hiding this comment

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

Please don't import multiple modules in a single line (other names are fine, but modules should be one per line).

Copy link
Member

Choose a reason for hiding this comment

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

I was wondering if we could lint this, but I presume not an easy to figure out what's a module and and object here?

Copy link
Member

@Oberon00 Oberon00 Aug 28, 2019

Choose a reason for hiding this comment

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

@toumorokoshi Since you approved the PR but made this comment, I'm not sure what's your opinion on such imports? Do you prefer importing multiple submodules on the same line or each on their own line.

Ad linters: I think pylint knows quite a bit about the meaning of these names. There is no check against it though.

Copy link
Member

@toumorokoshi toumorokoshi Aug 31, 2019

Choose a reason for hiding this comment

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

My opinion isn't strong enough for me to argue the point, but I actually prefer multiple per line. My personal favorite way to refer to an import is Rust syntax, which enables a lot of flexibility with a visually intuitive ident-as-namespace:

https://doc.rust-lang.org/reference/items/use-declarations.html

I think for those who don't use an ide, importing modules exclusively is really helpful. It makes it easy to see what belongs to a module vs what is a local reference:

foo = Foo()
other_object = other_module.Bar()

Once I know it's referenced locally or imported, I then make the determination to go to the top of the file, or to search for the next reference. But I use the go-to-definition functionality of my ide, and so the organization doesn't affect me either away.

The only thing I really dislike is star imports. But I think most people agree on that.

Copy link
Member

Choose a reason for hiding this comment

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

@Oberon00 it looks like the only way to use isort consistently with black is to allow grouped imports for module names.

Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

Actually, I realize this is just a question of stylistic preferences...

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

Thanks! I would suggest adding something in the squashed commit to reference the discussion around the isort changes, or file that in a separate issue. Somewhere where the discussion can be easily referenced.

@mauriciovasquezbernal
Copy link
Member

Would it be a good idea to add --diff to isort so ones knows what the code should look like?

@Oberon00
Copy link
Member

@mauriciovasquezbernal That sounds very useful but should be a separate PR.

These changes follow up the "Fix and improve tests for Python != 3.7" PR.

The multi_line_output was already set to 3 in the
"Add initial black formatting" PR, so after rebasing to master
this commit contains only comment that describes a magic number
from the isort configuration file.

Corresponding PR:

 - open-telemetry#109

Related discussions:

 - open-telemetry#95 (comment)

 - open-telemetry#95 (comment)
@Jamim Jamim force-pushed the feature/update-isort-settings branch from 5154805 to eaf0118 Compare August 30, 2019 21:27
@Jamim Jamim changed the title Update the isort multi line output setting Describe isort's multi_line_output setting Aug 30, 2019
@Jamim
Copy link
Contributor Author

Jamim commented Aug 30, 2019

Hi there,
I've updated the commit message and the description of this PR.

@Oberon00 Oberon00 added the style label Sep 2, 2019
Copy link
Member

@c24t c24t left a comment

Choose a reason for hiding this comment

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

Looks fine to me as a description of the current config.

@Oberon00 Oberon00 merged commit ec0dfb4 into open-telemetry:master Sep 5, 2019
@Jamim Jamim deleted the feature/update-isort-settings branch September 6, 2019 20:57
NathanielRN pushed a commit to NathanielRN/opentelemetry-python-contrib-1 that referenced this pull request Oct 20, 2020
These changes follow up the "Fix and improve tests for Python != 3.7" PR.

The multi_line_output was already set to 3 in the
"Add initial black formatting" PR, so after rebasing to master
this commit contains only comment that describes a magic number
from the isort configuration file.

Corresponding PR:

 - open-telemetry/opentelemetry-python#109

Related discussions:

 - open-telemetry/opentelemetry-python#95 (comment)

 - open-telemetry/opentelemetry-python#95 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants