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

Option to transform short circuit to if condition #765

Merged
merged 4 commits into from
Oct 31, 2023
Merged

Option to transform short circuit to if condition #765

merged 4 commits into from
Oct 31, 2023

Conversation

domluna
Copy link
Owner

@domluna domluna commented Oct 5, 2023

implements what's discussed in #759

cc @chriselrod if you could try this out that would be great

Copy link
Contributor

@hyrodium hyrodium left a comment

Choose a reason for hiding this comment

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

Could you please update docs/src/index.md to list the new option?

Project.toml Outdated
@@ -1,7 +1,7 @@
name = "JuliaFormatter"
uuid = "98e50ef6-434e-11e9-1051-2b60c6c9e899"
authors = ["Dominique Luna <[email protected]>"]
version = "1.0.39"
version = "1.0.40"
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR adds a new option short_circuit_to_if, so it would be better to bump the minor version.

Suggested change
version = "1.0.40"
version = "1.1.0"

Copy link
Owner Author

Choose a reason for hiding this comment

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

by default it's off so from the user perspective there's no change implying there's not really a need to bump it. Also there's this compatibility thing with VSCode where bumping the minor version frequently leads to things being out of sync so it's easier to just bump the patch version.

Copy link
Contributor

Choose a reason for hiding this comment

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

by default it's off so from the user perspective there's no change implying there's not really a need to bump it.

Hmm, Julia packages should respect semantic versioning, so I believe bumping the patch version should imply a bug fix. (cf. Pkg.jl docs, semver docs)

Also there's this compatibility thing with VSCode where bumping the minor version frequently leads to things being out of sync so it's easier to just bump the patch version.

What is the reason behind this? I couldn't find the [compat] table in Project.toml files in the julia-vscode repository.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also there's this compatibility thing with VSCode where bumping the minor version frequently leads to things being out of sync so it's easier to just bump the patch version.

Is it because of this (julia-vscode/julia-vscode#3226 (comment))? If so, the problem about the JuliaFormatter's version in vscode is not related to bumping patch or minor version.

@chriselrod
Copy link
Contributor

Just tried it, and works great!

454                 (1 <= a <= j) || error("oops")    488                 if !((1 <= a <= j))
...                                                   489                     error("oops")
...                                                   490                 end
455

Only possible suggestion (other than documenting it), is in case of ||, maybe check if the condition is already wrapped in parenthesis so we avoid double-wrapping?
Not that big of a deal, though.
Happy to see this.

@domluna domluna merged commit 75c0758 into master Oct 31, 2023
56 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.

3 participants