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.

Sign up for GitHub

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

Jump to bottom

Fix vector nesting with SciMLStyle and yas_style_nesting = true #807

Merged
merged 7 commits into from
Feb 15, 2024
Merged

Fix vector nesting with SciMLStyle and yas_style_nesting = true #807

merged 7 commits into from
Feb 15, 2024

Conversation

efaulhaber
Copy link
Contributor

@efaulhaber efaulhaber commented Feb 12, 2024 •
edited
Loading

Before #792, default style p_vect and n_vect! have always been used in SciMLStyle, even with yas_style_nesting = true. I restored this behavior.

Closes #806.

efaulhaber
efaulhaber commented Feb 12, 2024
View reviewed changes
src/styles/sciml/nest.jl Outdated
@@ -183,7 +183,7 @@ for f in [
:n_curly!,
:n_macrocall!,
:n_ref!,
:n_vect!,
# :n_vect!, don't use YAS style vector formatting with `yas_style_nesting = true`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With yas_style_nesting = true, this just restores the original version before #792.

With yas_style_nesting = false, #792 forwarded n_vect! to _n_tuple! below. Now it is forwarded to the default style n_vect!. Is this correct?

Copy link
Owner

Choose a reason for hiding this comment

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

This would change the nesting behaviour imposed in #792 so that it would put all arguments on one line instead of trying to best fit the arguments among the fewest lines while trying to fit within the maximum margin.

The entire point of #792 was that the former behaviour was unwanted amongst people in the SciML organization.

Copy link
Owner

Choose a reason for hiding this comment

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

for vect specifically if this is fine then I'm ok with the PR but I'm not sure if there's a concensus on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I guess we want to best fit, but still allow the line break with vector definitions. I'll have another look tomorrow. Or can you point me in the right direction how to do that?

Copy link
Owner

Choose a reason for hiding this comment

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

but still allow the line break with vector definitions

can you give an example of what this means?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I meant that I would like to combine the vector definition style that I restored in this PR with the best fit feature you implemented.

@efaulhaber efaulhaber mentioned this pull request Feb 12, 2024
Closed
This was referenced Feb 12, 2024
Merged
Merged
@YingboMa YingboMa mentioned this pull request Feb 12, 2024
Merged
@efaulhaber
Copy link
Contributor Author

efaulhaber commented Feb 13, 2024 •
edited
Loading

Okay, so I tried to make the best fit feature work with yas_style_nesting = true, but I couldn't figure it out. This stuff is a bit too complicated.
Note that best fit with yas_style_nesting = true doesn't work on master, either.

julia> str = """
       test = (arg1, arg2, arg3, arg4, arg5)
       """;

julia> print(format_text(str, SciMLStyle(), yas_style_nesting = true, margin = 34))
test = (arg1, arg2, arg3, arg4,
        arg5)

So I guess this PR really only fixes the bug and doesn't break anything else. But it would be a nice feature to have best fit with yas_style_nesting = true, so I'll create an issue for that.

@efaulhaber
Copy link
Contributor Author

#808

@domluna
Copy link
Owner

domluna commented Feb 13, 2024 •
edited
Loading

It's called here https://github.com/domluna/JuliaFormatter.jl/blob/master/src/styles/sciml/nest.jl#L100-L121 which is dispatched as SciMLStyle. YAS style hasn't changed.

When yas_style_nesting=true the YASStyle dispatch is used instead which is why you're getting the old YAS behaviour.

I believe YAS folks would be fine with using the new nesting in YAS as well.

#792 (comment)

@efaulhaber
Copy link
Contributor Author

Then I suggest merging this PR for now, and I'll see if I can find time to make the best fit feature work with YAS in a new PR.

@efaulhaber efaulhaber mentioned this pull request Feb 14, 2024
Open
@efaulhaber
Copy link
Contributor Author

Update: This PR is messing up SciMLStyle.
master:

julia> print(format_text(str3, SciMLStyle(), margin = 32))
test = [arg1, arg2, arg3,
    arg4, arg5]

This PR:

julia> print(format_text(str3, SciMLStyle(), margin = 32))
test = [
    arg1,
    arg2,
    arg3,
    arg4,
    arg5
]

@efaulhaber efaulhaber closed this Feb 14, 2024
@efaulhaber efaulhaber reopened this Feb 14, 2024
@efaulhaber
Copy link
Contributor Author

Now, this PR should not be breaking any SciML style features. It should only affect formatting with yas_style_nesting = true.

efaulhaber
efaulhaber commented Feb 14, 2024
View reviewed changes
test/sciml_style.jl
Comment on lines +524 to +558
fstr = """
test = [arg1, arg2,
arg3,
arg4, arg5]
"""
@test format_text(str, SciMLStyle(), margin = 20) == fstr
# should be 19? might be a unnesting off by 1 error
# @test format_text(str, SciMLStyle(), margin = 19) == fstr

fstr = """
test = [
arg1, arg2,
arg3,
arg4, arg5)
body
end
"""
@test format_text(str, SciMLStyle(), margin = 24) == fstr
arg4, arg5]
"""
@test format_text(str, SciMLStyle(), margin = 19) == fstr
# should be 15? might be a unnesting off by 1 error
@test format_text(str, SciMLStyle(), margin = 16) == fstr

fstr = """
test = [
arg1, arg2,
arg3,
arg4,
arg5]
"""
@test format_text(str, SciMLStyle(), margin = 15) == fstr

fstr = """
function foo(
fstr = """
test = [arg1,
arg2,
arg3,
arg4,
arg5]
"""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These look a bit weird. But this is how it's formatted on master. I only added these tests here to check that best optimal nesting still works with vector definitions because my previous attempt broke this.

@efaulhaber efaulhaber requested a review from domluna February 14, 2024 13:59
@efaulhaber efaulhaber closed this Feb 14, 2024
@efaulhaber efaulhaber reopened this Feb 14, 2024
@domluna
Copy link
Owner

domluna commented Feb 14, 2024

looks good to me the nightly test failure is because of the new Memory type. I'm not sure what that's about.

@efaulhaber
Copy link
Contributor Author

Do we have to wait for the nightly tests, or can we merge and release before that?

@domluna
Copy link
Owner

domluna commented Feb 15, 2024

we can merge it

@domluna domluna merged commit 50f8e60 into domluna:master Feb 15, 2024
56 checks passed
@efaulhaber efaulhaber mentioned this pull request Feb 26, 2024
Draft
@efaulhaber efaulhaber mentioned this pull request Sep 10, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@domluna domluna Awaiting requested review from domluna

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

#792 breaks vector definitions with SciMLStyle and yas_style_nesting
2 participants
@efaulhaber @domluna