Code review request: codeowners package

Questions / Help
1

Hello, I’m new to Elixir and created this package, which is the adaptation of some Ruby code I wrote, as a learning exercise: codeowners | Hex

I’m looking for feedback on:

  • How to make this code more idiomatic
  • More appropriate functions/modules to use
  • Best practices for libraries

Feedback is welcome here or in Github

1 Like
2

Hi, @reid-rigo :wave: and welcome!

I’d point you to this style guide: GitHub - christopheradams/elixir_style_guide: A community driven style guide for Elixir

Use the __MODULE__ pseudo variable when a module refers to itself.

It might look weird at first (becuase of double underscores and all caps) but it’s very common to refer to the module itself.

2 Likes
3

Some thoughts, in no particular order:

  • rule_for_path could be slightly more efficient if the Enum.reverse was moved into build to avoid doing the same computation more than once

  • the typespec for Codeowners.Rule is not satisfied by the default values in the defstruct. rule_for_path can return a struct containing unexpected types in the “no rule matched” case.

  • the use of File.cwd! in build is odd; consider allowing callers to either supply a root directly or as an override. To reduce the need for most callers to pass a root, load could do some guessing based on if the supplied file is in a doc or .github directory.

1 Like
4

I’m not seeing the full benefits of __MODULE__. It appears to help for future refactoring, but what about all of the references in docs?

5

rule_for_path could be slightly more efficient if the Enum.reverse was moved into build to avoid doing the same computation more than once

I’m really torn on this one. Reversing appears to take a few microseconds, and so for a CODEOWNERS file of significant size, it looks like most of the rule searching time will be spent in Regex.match?

Will ppl be surprised to load their CODEOWNERS and then see all of their rules in reverse order? I could reverse the list again for inspect purposes, but that feels like shenanigans.

the typespec for Codeowners.Rule is not satisfied by the default values in the defstruct. rule_for_path can return a struct containing unexpected types in the “no rule matched” case.

Thank you, fixed.

the use of File.cwd! in build is odd; consider allowing callers to either supply a root directly or as an override. To reduce the need for most callers to pass a root, load could do some guessing based on if the supplied file is in a doc or .github directory.

The ! definitely feels gross, though an exception appears to be a truly rare circumstance. I like your idea of doing some ‘guessing’ - Github specifies 3 possible locations for the file so it wouldn’t be hard to figure out which one was used and derive root from that.

Edit: Because Path.expand uses File.cwd!, I’m not sure there’s anything to be gained here. People should be able to call Codeowners.load(".github/CODEOWNERS")

6

What problems does using __MODULE__ introduce for you in docs?

7

What problems does using __MODULE__ introduce for you in docs?

Based on a suggestion in another thread I tried #{__MODULE__} , but that outputs Elixir.Codeowners when I run mix docs.

8

For me it’s not about refactoring—a global rename is a global rename—it’s about ease of reading. When I see __MODULE__ I immediately think “local.” Seeing a fully-qualified self-referencing module name always causes a bit of a jarring pause where I have to go and confirm that it is indeed referencing itself. This is indeed on the minor end of readability issues and is not a hill I’d die on. I do, however, vehemently disagree with the advice in that guide to use an alias if you want “a prettier name” :face_with_raised_eyebrow: If it weren’t for the few legitimate uses cases it has, I wouldn’t be upset if the :as option for alias were removed from the language.

4 Likes
9

One of the point you were asking:

  • How to make this code more idiomatic

So I just pointed to what seemed more idiomatic to me :slightly_smiling_face:

Based on a suggestion in another thread I tried #{__MODULE__} , but that outputs Elixir.Codeowners when I run mix docs.

So it does with any module name, such as: #{Logger} would print "Elixir.Logger". If you want to nicely disregard the prefix Elixir. you could inspect/1 it:

"#{inspect(__MODULE__)}"

See example: elixir/lib/elixir/lib/kernel/utils.ex at c5816a227f5ad295ccbfcc5571308f76190d6bfe · elixir-lang/elixir · GitHub

2 Likes
10

Thanks for helping me work through the __MODULE__ stuff. Updated in this commit.

11

the use of File.cwd! in build is odd; consider allowing callers to either supply a root directly or as an override. To reduce the need for most callers to pass a root, load could do some guessing based on if the supplied file is in a doc or .github directory.

I made a change to accept root as an option in this commit