Skip to content

Conversation

@igorp-collabora
Copy link

@igorp-collabora igorp-collabora commented Aug 1, 2025

Work in progress.

Files type hints added to:

  • src/configobj/validate.py
  • src/configobj/__init__.py
  • Add py.typed marker

CI stages added to verify typing:

  • Mypy. Mypy has to be version 1.16 at it is the last version that can type check Python 3.8.

arg_match = self._matchfinder.match(arg_string)
if arg_match is None:
# Bad syntax
raise VdtParamError('Bad syntax in check "%s".' % check)
Copy link
Author

Choose a reason for hiding this comment

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

I believe there is a real bug here:

src/configobj/validate.py:643: error: Missing positional argument "value" in call to "VdtParamError"  [call-arg]
                    raise VdtParamError('Bad syntax in check "%s".' % check)
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Found 1 error in 1 file (checked 1 source file)

VdtParamError takes 2 arguments to its initializer but only 1 is passed here. I haven't changed the code as it probably belongs to another PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I agree - it would be great if you could submit a separate PR for this.

Copy link
Author

Choose a reason for hiding this comment

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

Sure.

Copy link
Author

Choose a reason for hiding this comment

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

Hmmm... After some investigation I believe this was already fixed in the master branch but not release branch: dff3ca0

I am not entirely sure what is the relationship between those two branches.

@igorp-collabora
Copy link
Author

igorp-collabora commented Aug 1, 2025

The dict[..., ...] syntax was added in the Python 3.9 but from __future__ import annotations should prevent the code from evaluating on older Python versions. (older versions used typing.Dict[..., ...])

"""An unknown check function was requested"""

def __init__(self, value):
def __init__(self, value: Any):
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no value in adding "Any" annotations; can we be more specific here? Even just having a "Value" type alias might be useful for readability - and there are certain constraints on Values I think.

Copy link
Author

Choose a reason for hiding this comment

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

The value will be converted to a string anyway:

ValidateError.__init__(self, 'the check "%s" is unknown.' % (value,))

The object can be used here because any object can be converted to string: https://github.com/python/typeshed/blob/2888ec7011f5f2623fd3f3718d4668311932bb30/stdlib/builtins.pyi#L123

My plan is to add type hints with as minimal changes to the code as possible. Because of dynamic nature of existing code the use of Any is basically required. If you look at how serialization libraries are typed in standard library, all of them return Any when serialising or deserialising data: https://github.com/python/typeshed/blob/2888ec7011f5f2623fd3f3718d4668311932bb30/stdlib/json/__init__.pyi#L60

@igorp-collabora
Copy link
Author

I added very early __init__.py. This file is pretty large. I will refine them in the following days.

def getObj(s) -> Any:
global compiler
if compiler is None:
import compiler
Copy link
Author

Choose a reason for hiding this comment

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

I have no idea what this compiler module is. Nothing on PyPI matches it.

pass


class Builder(object):
Copy link
Author

@igorp-collabora igorp-collabora Aug 4, 2025

Choose a reason for hiding this comment

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

I don't believe this class is actually used anywhere?:

$ rg '_builder' src/
src/configobj/__init__.py
197:_builder = Builder()

def _interpolate(self, key: str, value: Any) -> Any:
try:
# do we already have an interpolation engine?
engine = self._interpolation_engine
Copy link
Author

Choose a reason for hiding this comment

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

I believe there might be a bug in this function. First the name = self.main.interpolation where interpolation is a bool. If interpolation then name will be assigned a string and .lower() will be called. However, if interpolation then name will remain a bool that does not have .lower() method.

I belive what prevents it is that __getitem__ checks for self.main.interpolation before calling _interpolate but if this check was omitted this function would definitely raise an error.

def _decode(self, infile, encoding):
def _decode(self, infile: str | bytes | list[bytes] | list[str] | tuple[str, ...], encoding: str | None) -> list[str]:
"""
Decode infile to unicode. Using the specified encoding.
Copy link
Author

Choose a reason for hiding this comment

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

There is an issue with this function. It says that it will decode file to unicode (str) but if encoding is None it will happily return bytes. encoding can be None because self.encoding can also be None.

self.configspec = None
self.initial_comment: list[str] = []
self.final_comment: list[str] = []
self.configspec: ConfigObj | None = None
Copy link
Author

Choose a reason for hiding this comment

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

There are a lot of type checking errors because self.configspec can be None. The None is not being checked for.

@igorp-collabora
Copy link
Author

I reduced number of type errors from over 300 to 44 but it is still pretty ugly. Here is the mypy output: https://gist.github.com/igorp-collabora/5eaa20e23b063d04f986d08d53d7c29a

@jelmer
Copy link
Collaborator

jelmer commented Sep 28, 2025

Hi @igorp-collabora , are you still working on this?

@igorp-collabora
Copy link
Author

igorp-collabora commented Sep 29, 2025

Hi @igorp-collabora , are you still working on this?

I ran in to some questions that I didn't have answers for:

  1. What is the relationship between release and master branches. For example, dff3ca0 bug fix is in master but not release. Should PR target master or release?
  2. What is this whole compiler and getObj thing that does not seems to be reachable? Can it be removed? (this will simplify type hint)

@jelmer
Copy link
Collaborator

jelmer commented Sep 30, 2025

Hi @igorp-collabora , are you still working on this?

I ran in to some questions that I didn't have answers for:

1. What is the relationship between `release` and `master` branches. For example, [dff3ca0](https://github.com/DiffSK/configobj/commit/dff3ca058d1d98f3078c17836435ca5af3a8c4a6) bug fix is in master but not release. Should PR target master or release?

Please target master. I'm working to simplify the branch situation.

2. What is this whole `compiler` and `getObj` thing that does not seems to be reachable? Can it be removed? (this will simplify type hint)

We can remove this. The compiler module used to be shipped with Python 2 (prior to 2.7?). Since we only support Python 3, we can drop it.

@igorp-collabora
Copy link
Author

Looks like master branch has already removed the compiler module. I will see if I can rework this MR to target master.

@igorp-collabora igorp-collabora changed the base branch from release to master October 17, 2025 15:03
@igorp-collabora
Copy link
Author

I've changed the target of this PR to master branch. There is still some difference between release and master .

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

Successfully merging this pull request may close these issues.

2 participants