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

V2 Fixes #883

Merged
merged 7 commits into from
Dec 26, 2024
Merged

V2 Fixes #883

merged 7 commits into from
Dec 26, 2024

Conversation

domluna
Copy link
Owner

@domluna domluna commented Oct 19, 2024

fixes #878

     open(path, "w") do io
-        TOML.print(io, data)
+        return TOML.print(io, data)
     end

The return is no longer added for do expressions.

@ararslan
Copy link

The return is no longer added for do expressions.

Is it not possible to support that? It's a requirement for YASGuide.

@domluna
Copy link
Owner Author

domluna commented Oct 29, 2024

The return is no longer added for do expressions.

Is it not possible to support that? It's a requirement for YASGuide.

Is the example above, with the return added, not a dealbreaker for YAS? It's really tricky to only add return in cases where not nothing is returned. I think in v1 this was broken in a bunch of ways but functional enough that it worked for most cases. In v2 this was fixed but the downside is that you get return TOML.print(io, data).

@ararslan
Copy link

It's really tricky to only add return in cases where not nothing is returned.

I'm not sure about the other style guides but at least for YAS the return is always required, even if it's nothing. In that example, return TOML.print(io, data) is correct for YAS. I'd probably personally write it as separate TOML.print(io, data) + return nothing but that's functionally equivalent and I wouldn't expect a formatter to be able to know that TOML.print is used for its side effects (writing to io) and not for its return value. Sounds like it's fully correct for YAS in v2 then?

@domluna
Copy link
Owner Author

domluna commented Nov 18, 2024

It's really tricky to only add return in cases where not nothing is returned.

I'm not sure about the other style guides but at least for YAS the return is always required, even if it's nothing. In that example, return TOML.print(io, data) is correct for YAS. I'd probably personally write it as separate TOML.print(io, data) + return nothing but that's functionally equivalent and I wouldn't expect a formatter to be able to know that TOML.print is used for its side effects (writing to io) and not for its return value. Sounds like it's fully correct for YAS in v2 then?

I'll put this back in for YAS style.

I wouldn't expect a formatter to be able to know that TOML.print is used for its side effects (writing to io) and not for its return value.

right, we would need to do side effect analysis which is out of scope for a formatter from my PoV.

@domluna
Copy link
Owner Author

domluna commented Dec 26, 2024

Merging this to master, still some things we can do but this had most of the fixes in.

@domluna domluna marked this pull request as ready for review December 26, 2024 19:08
@domluna domluna merged commit bec82d1 into master Dec 26, 2024
13 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.

New formatting turns optional positional argument into keyword argument
3 participants