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

Dirty macro warning + auto arity support #110

Merged
merged 11 commits into from
Feb 26, 2024
Merged

Dirty macro warning + auto arity support #110

merged 11 commits into from
Feb 26, 2024

Conversation

bartkrak
Copy link
Contributor

@bartkrak bartkrak self-assigned this Feb 21, 2024
@mat-hek
Copy link
Member

mat-hek commented Feb 21, 2024

@bartkrak could we also support passing a function without arity, that would match any arity? For example dirty :cpu, [:my_function, :another_function]

@bartkrak
Copy link
Contributor Author

@mat-hek It's done. Now you can use macro in two ways:
dirty :cpu, [:something, :another_one] - without supplying arity, it will automatically determine correct one (assuming there is no way to define a multi-arity function in unifex, we talked about this)
dirty :cpu, something: 0, another_one: 1 - the old way for backwards compatibility.
In both cases if you misspell name of the function or provide incorrect arity you will get a nice warning.

@bartkrak bartkrak changed the title Dirty macro warning Dirty macro warning + auto arity support Feb 22, 2024
config
|> Keyword.get_values(:dirty_functions)
|> List.flatten()
|> Enum.map(fn {dirty_func, type} ->
Copy link
Member

Choose a reason for hiding this comment

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

instead of map and reject, use flat_map

Comment on lines 93 to 104
{dirty_name, dirty_arity} =
cond do
is_tuple(dirty_func) ->
dirty_func

is_atom(dirty_func) ->
{dirty_func, nil}
end

{matched_name, matched_arity} = List.keyfind(functions_arity, dirty_name, 0, {nil, nil})

if matched_name == dirty_name and (matched_arity == dirty_arity or dirty_arity == nil) 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
{dirty_name, dirty_arity} =
cond do
is_tuple(dirty_func) ->
dirty_func
is_atom(dirty_func) ->
{dirty_func, nil}
end
{matched_name, matched_arity} = List.keyfind(functions_arity, dirty_name, 0, {nil, nil})
if matched_name == dirty_name and (matched_arity == dirty_arity or dirty_arity == nil) do
dirty_name =
case dirty_func do
{name, _arity} -> name
name -> name
end
{matched_name, matched_args} = List.keyfind(functions_args, dirty_name, 0, {nil, nil})
if dirty_func in [matched_name, {matched_name, length(matched_args)}] do

slightly more idiomatic, and you don't need functions_arity

|> Enum.unzip()

{functions_args, functions_arity} = Enum.unzip(functions_args_arity)
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
{functions_args, functions_arity} = Enum.unzip(functions_args_arity)
functions_arity = Enum.map(functions_args, fn {name, args} -> {name, Enum.count(args)} end)

You don't have to create functions_args_arity variable, you can just create function_args the way it has been done before your changes and create functions_arity with a simple Enum.map, without second call of Enum.unzip, IMO it will simplify your code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after changes suggested by @mat-hek this is no longer necessary

@FelonEkonom FelonEkonom self-requested a review February 22, 2024 16:42
Comment on lines 95 to 98
{matched_name, matched_args} = List.keyfind(functions_args, dirty_name, 0, {nil, nil})

if matched_name != nil and matched_args != nil and
dirty_func in [matched_name, {matched_name, length(matched_args)}] do
Copy link
Contributor Author

@bartkrak bartkrak Feb 22, 2024

Choose a reason for hiding this comment

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

@mat-hek I needed to add these nil checks to your suggestion, otherwise if List.keyfind() doesn't match length(matched_args) results in a crash

lib/unifex/specs.ex Outdated Show resolved Hide resolved
Co-authored-by: Mateusz Front <[email protected]>
@bartkrak bartkrak merged commit 65f7ecd into master Feb 26, 2024
3 checks passed
@bartkrak bartkrak deleted the dirty_macro_warn branch February 26, 2024 14:13
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.

4 participants