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

Added avoid repeating module name rule #136

Merged

Conversation

corroded
Copy link
Collaborator

@corroded corroded commented Mar 8, 2017

This is for #102

Copy link
Collaborator

@eksperimental eksperimental left a comment

Choose a reason for hiding this comment

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

For me this is somewhat misleading.
Why would you want to alias to a module within itself.
You should be using __MODULE__ instead

README.md Outdated
@@ -774,6 +774,32 @@ Translations of the guide are available in the following languages:
end
```

* <a name="avoid-repeating-aliased-module-names"></a>
If you plan to alias your modules, avoid using the same name consecutively.
This avoids any problems with [conflicting aliases](https://elixirforum.com/t/using-aliases-for-fubar-fubar-named-module/1723/3)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add a link to the beginning of the thread. not to the middle.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, pasted the wrong link lol. All good 👍

README.md Outdated

```elixir
# not preferred

Copy link
Collaborator

Choose a reason for hiding this comment

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

✂️

README.md Outdated
end

# somewhat preferred

Copy link
Collaborator

Choose a reason for hiding this comment

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

✂️

README.md Outdated
end

# preferred

Copy link
Collaborator

Choose a reason for hiding this comment

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

✂️

@corroded
Copy link
Collaborator Author

corroded commented Mar 8, 2017

@eksperimental addressed your comments :) Let me know if it needs more work. For examples, I just copied it from the original issue - I am still not that adept in Elixir to provide a better example (this is my way of studying/learning it)

README.md Outdated
@@ -774,6 +774,29 @@ Translations of the guide are available in the following languages:
end
```

* <a name="avoid-repeating-aliased-module-names"></a>
If you plan to alias your modules, avoid using the same name consecutively.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would replace "name" with "fragment"

README.md Outdated
alias Todo.Todo
end

# somewhat preferred
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is still not preferred. as you do not solve anything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you please help me with a better example for this? I was just following the original thread and the issue so not sure what a good example would be. To be honest, I would never name something consecutively like the example so really couldn't think of a use case where that is even possible.

README.md Outdated
alias Todo.List
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 don't think this is clearly explained.
The whole reason for not repeating fragments in the module name is because when they are aliased, it could be ambiguous to which module you are refereing to.

examples.

# not preferred
defmodule Todo.Todo do
  ...
end

defmodule AnotherModule do
  alias Todo.Todo
  # here it is ambigious to which one we refer when we call `Todo`
end

I think those would be more real life examples, as in the examples given the proper way to refer to module from within is using `__MODULE__` not matter what is called. the problem is when aliasing from another module. 

Copy link
Collaborator

@DavidAntaramian DavidAntaramian left a comment

Choose a reason for hiding this comment

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

I feel like this is not a solution. Overall, this is an example of a poor naming structure. This is solved by having a rational alias pattern, not by being tricky with alias calls. As Jose specifically says in the original forum post which elicited this change:

If the naming schema is confusing, I can't think of a better option besides changing the name. Why not call it Todo.List for the whole list and Todo.ListItem or similar for every element?

The solution, in my opinion, is to change the alias to something that won't cause confusion.

@corroded
Copy link
Collaborator Author

corroded commented Mar 8, 2017

@DavidAntaramian @eksperimental how about this:

  # not preferred
  defmodule Todo.Todo do
    ...
  end

  defmodule AnotherModule do
    alias Todo.Todo
    # here it is ambigious to which one we refer when we call `Todo`
  end

  # preferred
  defmodule Todo.List do
    ...
    # Basically, you should consider a better name for your module
    # if you ever find yourself duplicating fragments.
  end

I think if you're duplicating your module names/fragments, it is already a code smell. I totally agree with both your sentiments (including Jose's) that you should just pick a better name than duplicating something. It is too ambiguous and confusing.

@christopheradams
Copy link
Owner

I think it can be simplified. I think it just needs two brief examples with no additional comments of the style that is not preferred (Todo.Todo) and preferred (Todo.Item or Todo.Note). The reasons are clearer naming structure and no chance of ambiguous or confusing aliases.

@christopheradams
Copy link
Owner

christopheradams commented Mar 9, 2017

  • Avoid repeating fragments in module names and namespaces. This improves overall readability and eliminates ambiguous aliases.
# not preferred
defmodule Todo.Todo do
  ...
end

# preferred
defmodule Todo.Item do
  ...
end

@corroded
Copy link
Collaborator Author

corroded commented Mar 9, 2017

@christopheradams I've changed this to what you have said 👍 thanks!

@christopheradams christopheradams merged commit 481928e into christopheradams:master Mar 9, 2017
@corroded corroded moved this from In Review to Finished in Style guide Kanban board Mar 9, 2017
@corroded corroded removed this from Finished in Style guide Kanban board Mar 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants