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

Add config for code rewrites #4

Closed
wants to merge 6 commits into from
Closed

Add config for code rewrites #4

wants to merge 6 commits into from

Conversation

emkguts
Copy link
Collaborator

@emkguts emkguts commented Sep 18, 2024

Allow users to disable rewriting case to if since it can change behavior in very rare cases. The config is include_case_to_if. Also allow users to disable reordering config files since it can change behavior.

There is a very rare with rewrite that could change behavior (see issue), but it is so unlikely that I'm deeming it not worth spending time on.

@@ -174,6 +176,26 @@ defmodule Styler.Style.ConfigsTest do
)
end

test "does not reorder simple case if rewrite_if_to_unless is false" do
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
test "does not reorder simple case if rewrite_if_to_unless is false" do
test "does not reorder config files if reorder_configs is false" do

@@ -14,6 +14,8 @@ defmodule Styler.Style.ConfigsTest do

alias Styler.Style.Configs

require Logger
Copy link
Member

Choose a reason for hiding this comment

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

Left over from testing?

Comment on lines +91 to +92
defp maybe_exclude(list, _elem, true), do: list
defp maybe_exclude(list, elem, false), do: list -- [elem]
Copy link
Member

Choose a reason for hiding this comment

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

This will be fine but if there is a minute to spare, I'd suggest reviewing to see if there is a way to avoid a boolean argument. It tends to require much more tracing to understand what is going on.

Also, the value to the setting is only checked for nil when inserting which means there could be potentially be non-boolean values here which would break. If we keep this, we should remove the explicit false match

@emkguts emkguts force-pushed the line-length branch 9 times, most recently from fdce3e5 to da4fd4f Compare September 19, 2024 07:22
Base automatically changed from line-length to main September 19, 2024 07:25
@emkguts emkguts marked this pull request as draft September 19, 2024 07:28
@emkguts emkguts closed this Sep 26, 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.

2 participants