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

Correct the docstring to state that dither is enabled by default #14

Merged
merged 1 commit into from
Oct 5, 2024
Merged

Correct the docstring to state that dither is enabled by default #14

merged 1 commit into from
Oct 5, 2024

Conversation

shanecranor
Copy link
Contributor

@shanecranor shanecranor commented Oct 4, 2024

closes #13

@shanecranor
Copy link
Contributor Author

Confused on why the optional parameters are set to None and then set conditionally, rather than setting them directly to the defaults as well.

I can submit a PR to fix this if you think it would be a good idea.

That would be

        invert: bool = False,
        dither: bool = True,

instead of

invert: bool = None
dither: bool = None

.....

if dither is None:
    dither = True
  
      

@miikanissi
Copy link
Owner

Thanks for raising the issue and fixing this mistake!

I prefer to explicitly initialize the parameters as None in the constructor. This is helpful for mutable objects like lists and dictionaries and it also makes it easier to add more complex logic for setting default values for example the format parameter depends on compression_type here:

if format is None:
if compression_type is None:
format = "ASCII"
else:
format = self._compression_type_to_format(compression_type)
self.format = format.upper()

So for the sake of consistency every optional parameter is initialized as None.

@miikanissi miikanissi merged commit c9a8c00 into miikanissi:master Oct 5, 2024
6 checks passed
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.

Docstring is incorrect for dither
2 participants