Skip to content

Use constants defined in astroid.const where applicable #1372

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

Closed
wants to merge 2 commits into from

Conversation

DanielNoord
Copy link
Collaborator

Steps

  • Write a good description on what the PR does.

Description

Something I had on my mind for some time.

Type of Changes

Type
βœ“ πŸ”¨ Refactoring

Related Issue

@DanielNoord DanielNoord added the Maintenance Discussion or action around maintaining astroid or the dev workflow label Jan 27, 2022
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

I think this is nice change and also a small perf improvement but also that @cdce8p was against it because it makes it harder to detect sys guard. (We reverted some of it before). I don't have strong opinion about this but I also don't want to enter an edit war.

@DanielNoord
Copy link
Collaborator Author

I left occurrences for imports and redefinition of methods for specific versions, because of that issue. Of course we could still decide against it. I don't mind leaving as is.

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.10.0 milestone Jan 28, 2022
@Pierre-Sassoulas
Copy link
Member

I left occurrences for imports and redefinition of methods for specific versions, because of that issue.

Then I think we could merge if Marc do not react before 2.10.

@cdce8p
Copy link
Member

cdce8p commented Jan 30, 2022

I left occurrences for imports and redefinition of methods for specific versions, because of that issue.

Then I think we could merge if Marc do not react before 2.10.

Sorry guys! I needed some time to work on different things and just didn't look into any pylint or astroid issues as my backlog was getting larger. Am slowly catching up now, although I probably won't have much free time the next few months. If I'm unresponsive on Github you can usually still reach me on Discord, fyi.

I think this is nice change and also a small perf improvement but also that @cdce8p was against it because it makes it harder to detect sys guard.

Both mypy and pyright know how to handle sys guards. They don't have any logic for arbitrary constant guards. @DanielNoord You mentioned it yourself that you left some sys guards in-place because of it. Thus, I would still recommend against this change.

If performance is that important to you, I would suggest moving the sys guards one level up. E.g. for the TreeRebuilder instead of doing the checks inside the individual methods, define the methods itself conditionally. That way the check would only be run once on load, instead of for each method call.

There are some huge drawbacks though. By definition, you'll need to copy a lot of code. With that it will be much more difficult to maintain. That's why I didn't do it when I added the guards for end_lineno and end_col_offset. IMO it's just not worth it. At some point even Python 3.7 is deprecated and the problem resolves itself.

@DanielNoord
Copy link
Collaborator Author

Sorry guys! I needed some time to work on different things and just didn't look into any pylint or astroid issues as my backlog was getting larger. Am slowly catching up now, although I probably won't have much free time the next few months. If I'm unresponsive on Github you can usually still reach me on Discord, fyi.

As always, no worries! If you have any time to spare on astroid or pylint issues I think your input would be most valuable on the currently opened astroid PRs. There are both some crucial changes to distutils handling as well as the NamedTuple instance and mirroring ast APIs which would benefit from additional reviews.

At some point even Python 3.7 is deprecated and the problem resolves itself.

Fine with me. I just thought I would propose the change and start the discussion. Let's not do this! Thanks @cdce8p for the input! πŸ˜„

@DanielNoord DanielNoord deleted the constants branch January 30, 2022 22:34
@cdce8p
Copy link
Member

cdce8p commented Jan 30, 2022

The distutils PRs are still on my list. I've also seen a notification about ast. I don't want to make any promises, but just maybe I'll get to them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion πŸ€” Maintenance Discussion or action around maintaining astroid or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants