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

Respect existing capitalization in defmodule completion #819

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mat-hek
Copy link

@mat-hek mat-hek commented Aug 16, 2024

closes #786

closest_name_split =
project_modules
|> Enum.map(fn {module_string, _already_loaded?} ->
with "Elixir." <> ms <- module_string, do: ms
Copy link
Author

Choose a reason for hiding this comment

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

not sure if this is the way 🤔

@zachallaun
Copy link
Collaborator

zachallaun commented Sep 6, 2024

Thanks for the PR! Sorry about the delay in reviewing it -- I think everyone has been in a bit of a holding pattern after the announcement of the combined language server effort. But I think we should move on this and not let it languish!

I can't fully review this right now, but I did have one thought:

What if, instead of looking for the longest existing prefix, we looked at existing capitalization on a per-segment basis? For instance, imagine you have a Foo.HTTP and now you're adjusting capitalization for Bar.Http. Currently, it wouldn't be adjusted because of the prefix match. However, we could store a map of segments, so it would have %{"foo" => "Foo", "http" => "HTTP"} and then you just iterate over the list of segments in the module you're adjusting and do something like Map.get(case_map, lower_segment, original_segment).

There may be some edge cases I've not thought of, and we'd probably want to limit the case map to only modules in the current application as opposed to those in all dependencies. Thoughts? @scohen?

@mat-hek
Copy link
Author

mat-hek commented Sep 6, 2024

What if, instead of looking for the longest existing prefix, we looked at existing capitalization on a per-segment basis?

Good idea. Currently, having Foo.HTTP we'd also adjust Foo.Httpserver -> Foo.HTTPServer. To preserve this, we can do per-prefix matching on the segments ;)

Comment on lines +208 to +233
def adjust_name_capitalization(name) do
name_downcase = String.downcase(name)
name_split = String.split(name, ".")
project_modules = all_modules()

closest_name_split =
project_modules
|> Enum.map(fn {module_string, _already_loaded?} ->
with "Elixir." <> ms <- module_string, do: ms
end)
|> Enum.max_by(fn module_string ->
:binary.longest_common_prefix([String.downcase(module_string), name_downcase])
end)
|> String.split(".")

prefix =
closest_name_split
|> Enum.zip(name_split)
|> Enum.take_while(fn {closest_name, name} ->
String.downcase(closest_name) == String.downcase(name)
end)
|> Enum.map(fn {closest_name, _name} -> closest_name end)

suffix = Enum.drop(name_split, length(prefix))
Enum.join(prefix ++ suffix, ".")
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

I made a separate comment about a potentially different way of adjusting capitalization, but if we stick with this approach, I think this can be done with one iteration over existing modules:

Suggested change
def adjust_name_capitalization(name) do
name_downcase = String.downcase(name)
name_split = String.split(name, ".")
project_modules = all_modules()
closest_name_split =
project_modules
|> Enum.map(fn {module_string, _already_loaded?} ->
with "Elixir." <> ms <- module_string, do: ms
end)
|> Enum.max_by(fn module_string ->
:binary.longest_common_prefix([String.downcase(module_string), name_downcase])
end)
|> String.split(".")
prefix =
closest_name_split
|> Enum.zip(name_split)
|> Enum.take_while(fn {closest_name, name} ->
String.downcase(closest_name) == String.downcase(name)
end)
|> Enum.map(fn {closest_name, _name} -> closest_name end)
suffix = Enum.drop(name_split, length(prefix))
Enum.join(prefix ++ suffix, ".")
end
def adjust_name_capitalization(name) do
name_downcase = String.downcase(name)
{prefix, byte_length} =
Enum.reduce(all_modules(), {"", 0}, fn
{"Elixir." <> existing, _loaded?}, {prefix, length} ->
new_length =
:binary.longest_common_prefix([String.downcase(existing), name_downcase])
if new_length > length do
{binary_slice(existing, 0, new_length), new_length}
else
{prefix, length}
end
_, acc ->
acc
end)
<<_::binary-size(byte_length), suffix::binary>> = name
prefix <> suffix
end

@zachallaun
Copy link
Collaborator

What if, instead of looking for the longest existing prefix, we looked at existing capitalization on a per-segment basis?

Good idea. Currently, having Foo.HTTP we'd also adjust Foo.Httpserver -> Foo.HTTPServer. To preserve this, we can do per-prefix matching on the segments ;)

Cool! Maybe the "existing capitalizations" map can store title case segments, so having an existing Foo.HTTPServer would store %{"foo" => "Foo", "http" => "HTTP", "server" => "Server"}. It looks like the following regex does the job of correctly splitting title case, then the operation just becomes a nested map where we map over module segments split on . and then over title case segments:

r = ~r"""
# last char was lower and next char is upper:
# FooBAR
#   ^
(?<=[a-z])(?=[A-Z])

# or
|

# last char was upper and next chars are upper followed by lower:
# FOOBar
#   ^
(?<=[A-Z])(?=[A-Z][a-z])
"""x

[
  "FooBarBaz",
  "FOOBarBaz",
  "FooBARBaz",
  "FooBarBAZ",
  "foobarbaz",
  "A",
  "AB",
  "Ab",
  "AbC",
  "aBC"
]
|> Enum.each(fn name ->
  name
  |> String.split(r)
  |> IO.inspect()
end)

@mat-hek
Copy link
Author

mat-hek commented Sep 30, 2024

So, I've been quite busy recently and it's not going to change in October, so it'll probably be a while till I sit to this again :( So if anyone would like to take it over, don't hesitate, just let me know ;) Or I'll get back to it when I'm available

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.

Respect existing capitalization in defmodule completion
2 participants