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

with true <- some_func incorrectly being rewritten #186

Closed
dvic opened this issue Aug 9, 2024 · 3 comments
Closed

with true <- some_func incorrectly being rewritten #186

dvic opened this issue Aug 9, 2024 · 3 comments
Labels
wontfix This will not be worked on

Comments

@dvic
Copy link

dvic commented Aug 9, 2024

Versions

  • Elixir: 1.17
  • Styler: 1.0

Example Input

# returns :ok || other_func()
with true <- some_func() do
  other_func()
end

def some_func do
  condition?() || :ok
end

Stacktrace / Current Behaviour

# returns nil || other_func()
if some_func() do
  other_func()
end
`
``
@novaugust
Copy link
Contributor

hey @dvic, thanks for the report. sorry styler introduced a bug in your program, but i've not convinced myself i want to change this behaviour.

there's a short nerdy essay that can be summed up as Use With Only When Nothing Else Suffices over in styler's docs, and that philosophy is kind of our guiding star.

styler errs on the side of readable/understandable code. so here, it's assuming that a function that returns a boolean is at least working in a truthy/falsey realm, and so is simplifying things into an if statement.

while i regret that this introduces a bug here, i also hope it shines a light on a potential refactor. you'll get the behaviour you're looking for by just adding a little to styler's rewrite:

if condition?(), 
  do: other_func(), 
  else: :ok

that way no one reading the code trips up assuming that some_func returning true on success means it returns false on failure (and you've got less code to maintain overall, though i realize you're just kicking a minimal example our way, and appreciate it =D)

tldr i'll sit on this for a while, but between the trade-offs i might prefer the bug introducing one.

@dvic
Copy link
Author

dvic commented Aug 9, 2024

I agree 100% with you that this pattern is not so nice and I already fixed the occurrences in our code :)

it's assuming that a function that returns a boolean is at least working in a truthy/falsey realm

But still chances are quite likely that the converted code is not the same right? Setting aside the :ok in my example, users could return false from such a function and then the rewritten code does the same thing right?

In any case, I'll let you sit on this one for a while, it's fixed in our codebase 👍🏼

@novaugust
Copy link
Contributor

novaugust commented Aug 9, 2024

But still chances are quite likely that the converted code is not the same right?

oh absolutely! there's a big header on the readme about that. there's more than a few ways that styler can change the semantics/behaviour of a program

!Styler can change the behaviour of your program!

here's a specific example of where it does, rewriting case to if (which is the same as what it did with your program but with sadder results due to the obscurity of with statements)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants