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

format with JuliaFormatter attempt 87 br #383

Closed
wants to merge 1 commit into from

Conversation

isaacsas
Copy link
Member

@domluna I tested your pr domluna/JuliaFormatter.jl#792

There still seem to be some issues with moving args that fit on one line across many lines, and with sub-lines of function signatures not getting indented. I'll tag them below.

Comment on lines +10 to 24
mathengine = MathJax3(
Dict(
:loader => Dict("load" => ["[tex]/require", "[tex]/mathtools"]),
:tex => Dict(
"inlineMath" => [["\$", "\$"], ["\\(", "\\)"]],
"packages" => [
"base",
"ams",
"autoload",
"mathtools",
"require",
],
),
))

Copy link
Member Author

Choose a reason for hiding this comment

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

expanding to more lines than needed

Comment on lines +29 to +32
clean = true,
doctest = false,
linkcheck = true,
warnonly = [:missing_docs],
Copy link
Member Author

Choose a reason for hiding this comment

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

same

Copy link

Choose a reason for hiding this comment

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

makedocs(
    sitename = "JumpProcesses.jl",
    authors = "Chris Rackauckas",
    modules = [JumpProcesses],
    clean = true, doctest = false, linkcheck = true, warnonly = [:missing_docs],
    format = Documenter.HTML(;
        assets = ["assets/favicon.ico"],
        canonical = "https://docs.sciml.ai/JumpProcesses/",
        prettyurls = (get(ENV, "CI", nothing) == "true"),
        mathengine,
    ),
    pages = pages,
)

in the most recent commit

Copy link

Choose a reason for hiding this comment

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

the unnesting could still be a little better though

Copy link
Member Author

Choose a reason for hiding this comment

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

That is looking better! I think if you can get it to not add the initial and final new lines so both parentheses are kept as originally placed it would be good.

Copy link

Choose a reason for hiding this comment

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

mathengine = MathJax3(
    Dict(:loader => Dict("load" => ["[tex]/require", "[tex]/mathtools"]),
        :tex => Dict("inlineMath" => [["\$", "\$"], ["\\(", "\\)"]],
            "packages" => [
                "base",
                "ams",
                "autoload",
                "mathtools",
                "require",
            ])))

makedocs(sitename = "JumpProcesses.jl",
    authors = "Chris Rackauckas",
    modules = [JumpProcesses],
    clean = true, doctest = false, linkcheck = true, warnonly = [:missing_docs],
    format = Documenter.HTML(; assets = ["assets/favicon.ico"],
        canonical = "https://docs.sciml.ai/JumpProcesses/",
        prettyurls = (get(ENV, "CI", nothing) == "true"),
        mathengine),
    pages = pages)

Copy link

Choose a reason for hiding this comment

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

mathengine = MathJax3(Dict(:loader => Dict("load" => ["[tex]/require", "[tex]/mathtools"]), could still be 1 line so might be the calc is slightly off but it's much better now regardless

Comment on lines +13 to +15
function reset_aggregated_jumps!(integrator, uprev = nothing;
update_jump_params = true,
kwargs...)
Copy link
Member Author

Choose a reason for hiding this comment

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

same

Comment on lines +35 to +37
function reset_aggregated_jumps!(integrator, uprev, cb::DiscreteCallback,
cbs...;
update_jump_params = true, kwargs...)
Copy link
Member Author

Choose a reason for hiding this comment

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

same

Comment on lines +166 to +168
const JUMP_AGGREGATORS = (Direct(), DirectFW(), DirectCR(), SortingDirect(), RSSA(),
FRM(),
FRMFW(), NRM(), RSSACR(), RDirect(), Coevolve())
Copy link
Member Author

Choose a reason for hiding this comment

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

same

Copy link

Choose a reason for hiding this comment

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

so with a lot of these it's because it's fighting with join_lines_based_on_source=true

on this line in particular

const JUMP_AGGREGATORS = (Direct(), DirectFW(), DirectCR(), SortingDirect(), RSSA(), FRM(), FRMFW(), NRM(), RSSACR(), RDirect(), Coevolve())

would be output with =false

in the original source FRM() has a newline after it which is why it's kept. So I have to make the algo aware of that.

But I'd be curious to see what the format would be like in you do join_lines_based_on_source=false at least for the initial format and then once you get a split you're ok with turn it back on or something

Comment on lines +54 to +57
function cat_problems(
prob::DiffEqBase.AbstractSDEProblem,
prob_control::DiffEqBase.AbstractSDEProblem,
)
Copy link
Member Author

@isaacsas isaacsas Dec 19, 2023

Choose a reason for hiding this comment

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

This seems to be putting the ) on a new line and leaving it unindented when the discussion in that long issue was that it should just stay at the end of the last argument and sub-lines of the function signature should be double indented always.

Copy link

Choose a reason for hiding this comment

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

solved in most recent commit

@coveralls
Copy link

coveralls commented Dec 19, 2023

Pull Request Test Coverage Report for Build 7261713574

  • 107 of 128 (83.59%) changed or added relevant lines in 27 files are covered.
  • 4 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.008%) to 89.953%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/aggregators/directcr.jl 0 1 0.0%
src/aggregators/nrm.jl 1 2 50.0%
src/aggregators/rdirect.jl 1 2 50.0%
src/aggregators/sortingdirect.jl 1 2 50.0%
src/solve.jl 0 1 0.0%
src/spatial/spatial_massaction_jump.jl 6 7 85.71%
src/spatial/utils.jl 1 2 50.0%
src/aggregators/rssa.jl 0 2 0.0%
src/aggregators/rssacr.jl 2 4 50.0%
src/aggregators/ssajump.jl 8 10 80.0%
Files with Coverage Reduction New Missed Lines %
ext/JumpProcessFastBroadcastExt.jl 1 50.0%
src/problem.jl 1 65.28%
src/spatial/spatial_massaction_jump.jl 2 90.74%
Totals Coverage Status
Change from base Build 7210509298: 0.008%
Covered Lines: 2301
Relevant Lines: 2558

💛 - Coveralls

@domluna
Copy link

domluna commented Jan 4, 2024

@isaacsas i think most of the issues are solved now - aside from some off by one by 1 errors

@isaacsas
Copy link
Member Author

isaacsas commented Jan 4, 2024

Ok, cool thanks for letting me know. If I have a chance I’ll try to check it out tomorrow morning before I head on holiday for a while and give you an update.

@isaacsas isaacsas closed this Mar 20, 2024
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.

3 participants