While its possible booleans in general in an API may be bad, but the example given here doesn't prove it. As soon as I read that there was a need to support new output type, it was obvious an enum is needed. Putting in a boolean there didn't make sense to me at all.
It's obvious in hindsight, whereas when the API was first created, an enum was not needed because initially, there was no need for further support. In fact, in many cases, there is never any need for new output types, which then technically supports a boolean type (yet isn't necessarily the best choice still).
For me it was not in hindsight. As soon as I saw a boolean being used in your example, it was already weird for me. To me it sounded more like a hack which we usually do when we are crunched on time. An output type is just like a mime type, what I would have done was take the mime type as the param.
Yup - that's a great idea. Lots of ways to skin the cat here. My example isn't perfect, but one can make the argument that extending a function into multiple smaller functions that do similar things is even more unmaintainable.
Having to maintain 6 different API endpoints of get_llm_call_as_csv, get_llm_call_as_html, etc. is, in my opinion, even more unmaintainable than just using an enum in this case. That's 6 different API endpoints more to maintain compared to just 1, especially for functionality that is small enough that an enum can be the switch here.
Exactly. A function for each type of output and an abstraction that is being “called” in the main logic. This way you close the code for modification, but it is open for extension, because you can easily add a new output function, without modifying the existing logic.
I think you blame using bool values with bad example of usage. Your sample clearly need multiple options data structure not 2 states. Bools are in life and have usage of in programming for solving problems. So I do not think you are correct , bools are not trap.
In the database world, i usually don’t like using boolean. Prefer tinyint 1/0. Enum for database, not crazy about it. It requires an alter on table to change enum values. If table has 1000 rows, that’s fine, but 100M, different story.
I agree that boolean function parameters can be problematic because call sites using a bare `true` or `false` value can't be understood alone. But my first solution is to make the parameter *keyword-only* (rather than replacing the boolean with an enum), so that call sites *must* include the parameter name when making the call. In your example it would force calls to look like `call_llm(..., output_plaintext=True)`. Later, if the parameter needs to take more than 2 values, my second solution would be to then upgrade to an enum.
That's why this is already included as a linter rule in ruff (originally from flake8-boolean-trap)
https://docs.astral.sh/ruff/rules/boolean-type-hint-positional-argument/
Also with useful hint of making the boolean argument keyword-only
While its possible booleans in general in an API may be bad, but the example given here doesn't prove it. As soon as I read that there was a need to support new output type, it was obvious an enum is needed. Putting in a boolean there didn't make sense to me at all.
It's obvious in hindsight, whereas when the API was first created, an enum was not needed because initially, there was no need for further support. In fact, in many cases, there is never any need for new output types, which then technically supports a boolean type (yet isn't necessarily the best choice still).
For me it was not in hindsight. As soon as I saw a boolean being used in your example, it was already weird for me. To me it sounded more like a hack which we usually do when we are crunched on time. An output type is just like a mime type, what I would have done was take the mime type as the param.
That's a fair point :). I appreciate the feedback!
My example probably could have been better.
Why even use an enum? That's just asking one thing to do multiple things. One thing should do one thing.
Why not just have a function for each output type?
Yup - that's a great idea. Lots of ways to skin the cat here. My example isn't perfect, but one can make the argument that extending a function into multiple smaller functions that do similar things is even more unmaintainable.
Having to maintain 6 different API endpoints of get_llm_call_as_csv, get_llm_call_as_html, etc. is, in my opinion, even more unmaintainable than just using an enum in this case. That's 6 different API endpoints more to maintain compared to just 1, especially for functionality that is small enough that an enum can be the switch here.
That's fair. I want thinking about the endpoints. I agree there.
Either way, your point is very well made.
I avoid booleans as you recommend. I think the enum is a very underused structure.
And I despise the use of underscores in variable names. 🙃
Exactly. A function for each type of output and an abstraction that is being “called” in the main logic. This way you close the code for modification, but it is open for extension, because you can easily add a new output function, without modifying the existing logic.
Yes!
This article blames train as a mode of transportation when someone uses it to travel from Seattle to New York.
That's why this is already included as a linter rule in ruff (originally from flake8-boolean-trap)
https://docs.astral.sh/ruff/rules/boolean-type-hint-positional-argument/
Also with useful hint of making the boolean argument keyword-only
I think you blame using bool values with bad example of usage. Your sample clearly need multiple options data structure not 2 states. Bools are in life and have usage of in programming for solving problems. So I do not think you are correct , bools are not trap.
In the database world, i usually don’t like using boolean. Prefer tinyint 1/0. Enum for database, not crazy about it. It requires an alter on table to change enum values. If table has 1000 rows, that’s fine, but 100M, different story.
In the database world, boolean is not recommended either. I usually use tinyint, 1 or 0.
Enum, not crazy about it. Alter on table is required to change enum content. Not ideal.
I agree that boolean function parameters can be problematic because call sites using a bare `true` or `false` value can't be understood alone. But my first solution is to make the parameter *keyword-only* (rather than replacing the boolean with an enum), so that call sites *must* include the parameter name when making the call. In your example it would force calls to look like `call_llm(..., output_plaintext=True)`. Later, if the parameter needs to take more than 2 values, my second solution would be to then upgrade to an enum.
Ive read a book , an old one, it says pragmatic programmer. it is ahead of its time. :)
It’s indeed an improvement, but with enums you are breaking the Open/Closed Principle… which can be fixed with the Strategy pattern.