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

Remove deprected builtins in MODULE_BUILTINS #89

Open
cgahr opened this issue May 1, 2023 · 8 comments
Open

Remove deprected builtins in MODULE_BUILTINS #89

cgahr opened this issue May 1, 2023 · 8 comments

Comments

@cgahr
Copy link
Collaborator

cgahr commented May 1, 2023

_builtins.py defines MODEULE_BUILTINS.

The following elements are not found in import builtins; dir(builtins):

{
    "StandardError",
    "WindowsError",
    "_",
    "__annotations__",
    "__builtins__",
    "__cached__",
    "__file__",
    "__path__",
    "apply",
    "basestring",
    "buffer",
    "cmp",
    "coerce",
    "execfile",
    "exit",
    "file",
    "intern",
    "long",
    "quit",
    "raw_input",
    "reduce",
    "reload",
    "unichr",
    "unicode",
    "xrange",
}

My question is: Do we need these elements?

For example:

So my question is, do we need these elements?
Can we determine MODULE_BUILTINS dynamically?

@jgberry
Copy link
Collaborator

jgberry commented May 3, 2023

I originally intended to add testing for this in #85; however, it's more challenging than I original though. How do we determine if a builtin is deprecated or platform dependent? I couldn't find a simple way to determine this dynamically (i.e. at runtime). What's even more complicated is that not all builtins are present in the builtins module. For example, the dunders in the above list are often conditionally defined, as I mentioned in #85.

WindowsError is OS specific and should only be included if the OS is windows.

This is a no go. The platform you run ssort on should be completely independent of the platforms you can run your application on. For instance, a developer may personally develop on Linux, and therefore run ssort on Linux, but have their application target both Linux and Windows. This is a perfectly valid use case which we must anticipate.

Can we determine MODULE_BUILTINS dynamically?

As mentioned above, this would be difficult, if not impossible because of platform dependent builtins.

@bwhmather
Copy link
Owner

What would be the downside to simply removing the list of builtins and ignoring unresolved dependencies?

My feeling is that it was something that I introduced when I was trying to get ssort to do much more aggressive grouping. With the rules as they were then, a typo that resulted in a lookup failure would often result in a significant reshuffling. With the rules as they are now, I think the only case where an undesired reshuffling could happen is if we wanted to introduce a cycle.

On the plus side, unresolved dependency errors are endlessly infuriating during development as they block other auto-formatters from running.

@cgahr
Copy link
Collaborator Author

cgahr commented May 4, 2023

Using a function like this (test_builtins.py)

def test_no_additional_builtins():
    additional = MODULE_BUILTINS
    additional -= _get_builtins()
    additional -= _get_init_builtins()
    additional -= _get_main_builtins()
    additional -= _get_module_builtins()
    assert not additional

we can determine a list of builtins that are defined in MODULE_BUILTINS but not in "builtin" to python.

This is a complete list of additional elements. Note that all dunder methods are indeed builtins:

{
    'StandardError',
    'WindowsError',
    '_',
    'apply',
    'basestring',
    'buffer',
    'cmp',
    'coerce',
    'execfile',
    'file',
    'intern',
    'long',
    'raw_input',
    'reduce',
    'reload',
    'unichr',
    'unicode',
    'xrange'
 }

Basically all of these are relics of Python 2 and no longer in use:

Thus, only _ remains.

_ placeholder variable

currently _ is considered to be a builtin since its generally used as a placeholder for unused variables. However, the following abominationcode is valid python code and does not get sorted correctly by ssort:

def fun():
    return _


def _():
    return ""

So should we remove _?

In any case, I think the easiest way going format is the following:

  • remove all builtins found in the builtins module
  • hard the "exotic" builtins

This way we would have the best of both worlds I think.

@bwhmather
Copy link
Owner

Python 2 builtins also important for the same reason as platform specific ones: code can run under different versions of python as well as on different platforms. If we have a list of builtins then it should list all possible builtins.

@bwhmather
Copy link
Owner

So should we remove _?

Is it ever safe to dereference _ without it being explicitly bound? If not then the answer is probably yes. There is an element of Chesterton's fence here though.

@bwhmather
Copy link
Owner

I still have an deep seated aversion to deleting stuff I've spent a lot of time on so this hurts, but I think this is the better solution: #96

@cgahr
Copy link
Collaborator Author

cgahr commented May 11, 2023

Python 2 builtins also important for the same reason as platform specific ones: code can run under different versions of python as well as on different platforms. If we have a list of builtins then it should list all possible builtins.

I think the question then is: do we want to support legacy python 2 code snippets? There is at least on test in test_data.

I personally would argue that we are in 2023 and python 2 should not be used so remove those and call it a day.

@bwhmather
Copy link
Owner

I still have an deep seated aversion to deleting stuff I've spent a lot of time on so this hurts, but I think this is the better solution: #96

Based on discussion with you and @jgberry, have determined that removing the warnings in #96 is not sufficient to allow removing the builtins list safely.

Going to leave this open for a couple more years.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants