[discussion]: Avoiding the use of builtin function names as parameter names?

[joelostblom]

What would you like to discuss?

The metadata schema involves some keys that have the same name as Python builtin functions (e.g. type and format). This means that there is a chance for name collision when we convert these to parameter names for the helper classes. For example, within the class ResourceProperties, we could not use the Python functions type and format since they are shadowed by the parameter names. This also shows up as different syntax highlighting in the docs:

Maybe not a giant problem, but I thought I would document it. We have a similar situation in Altair converting from JSON schema keys to class parameters. There the solution was chosen to suffix names already in use with an underscore to avoid potential collisions (although it's not that pretty either), e.g.: https://github.com/vega/altair/blob/f23c1c8b9bb01d12b972609f7a112ced141c10ab/altair/vegalite/v6/api.py#L2766-L2772

[lwjohnst86]

We should as much as possible name the fields to match what is in the Data Package standard. Meaning not add _ to the end.

I'm having a hard time imagining how this issue might come up though. Could you give a potentially realistic example? Do you know if it is possible to prevent sub-methods of the dataclass from being used (we don't really want them anyway... also another reason I dislike methods on classes when the classes also hold data ... 😒 )

[joelostblom]

From a dev perspective, it could potentially convolute debugging and code maintenance, and it prevents from using the builtin functions inside the files that redefines those identifiers. Many linters flag it for these reasons; some more info in this article from Ruff.

From a user perspective, I don't think it could cause any issues.

Not the this is also a bit different than the as_ example in altair since as is a reserved keyword and thus impossible to redefine, whereas type etc are bult-in functions technically possible to redefine (with the above caveats).

[lwjohnst86]

I'm still not understanding, could you give a code example of an issue that might come up? Because the link you sent refers to variables, not class attributes. And type() is a function not a method so that class doesn't matter, and format() is a method on strings, which isn't an issue for our classes. But like I said, I'm not fully understanding the issue.

I sense that as long as we know what it is, it shouldn't be too much of a problem (it's not like the Sprout codebase is so large that we can't scan it manually if need be).

[joelostblom]

Because the link you sent refers to variables, not class attributes.

Wouldn't the issue be the same as in the Ruff example with the variables? If you try to use type/format inside ResourceProperties they would refer to the redefined version and not the builtin.

For example:

 from dataclasses import dataclass

@dataclass
class ResourceProperties:
    format: int = 4
    my_str: str = format(4)

TypeError: 'int' object is not callable

As I wrote above, I don't think this is a big issue, but could potentially lead to a some extra time spent debugging if a default well-known behavior of a built in function is overwritten (then again I don't think we are likely to use the overwritten built-in functions anywhere which is why I don't see it as a big issue).

[lwjohnst86]

For this example, we don't include any functions like that in the properties classes attributes. All default to None.

But as you said, this probably isn't a big issue. Thanks for making the issue, we can always refer back to it if it does become an issue ☺️