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 Style/ClassAndModuleChildren style: namespace #9887

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

markburns
Copy link
Contributor

@markburns markburns commented Jun 21, 2021

This adds support for a style wherein the concept of a module used as a namespace is kept separate from the definition of a
class or module itself.

I.e. making a distinction between a module to represent a software package, and a module to include behaviour.

Since the introduction of Zeitwerk in Rails, the benefits of retaining the nested style in terms of accurate constant lookup (when using Zeitwerk) have disappeared.

This makes the compact style more appealing for the following reasons:

  • Less low-information-value horizontal whitespace.
  • Less git churn when moving files around between differing namespace depths.
  • Easier scanning on the eyes when looking for the main constant.

The compact style suffers slightly from the following though:

  • Main constant definition in a file is far over to the right in heavily namespaced files.
  • Change to either the namespace or the constant name results in change to the same line.

The new proposed namespace style allows for

  • Conceptual separation of the namespace/package concept from the class or
    module definition.
  • File move refactors to a different namespace result in a diff that only touches
    the namespace constant part of the file.
  • Scanning by eye for the constant name is easier.
# nested style
module Foo
  module Bar
    module Baz
      module Qux
        module Quux
          module Quuz
            module Corge
              module Grault
                module Garply
                  module Waldo
                    module Fred
                      class Plugh
                      end
                    end
                  end
                end
              end
            end
          end
        end
      end
    end
  end
end
# proposed namespace style
module Foo::Bar::Baz::Qux::Quux::Quuz::Corge::Grault::Garply::Waldo::Fred
  class Plugh
  end
end
# compact style
class Foo::Bar::Baz::Qux::Quux::Quuz::Corge::Grault::Garply::Waldo::Fred::Plugh
end

Caveats

It may be nice to support using a Class as a namespace as a configurable option. This doesn't seem straightforward using static analysis though, so the results would be quite ugly.

Example if a Class could be a namespace:

module Foo::Bar::Baz
  class Qux
    module Quux::Quuz
      class Corge
        class Grault
          module Garply::Waldo::Fred
            class Plugh
            end
          end
        end
      end
    end
  end
end

I have chosen in this implementation to just reject using Classes as namespaces.

Note this PR depends upon a fix being in place for #9886 . Which I am happy to add in separately onto that PR once I've either had feedback about that needing a fix and/or had some pointers, or once I've worked out how to do it myself.

I have put a commit which inserts the invalid excess whitespace into the expectations in 55e8ae8

If #9886 is approved and merged, I can rebase against that as this PR depends upon that fix.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

end

def already_warned_about_namespace_in_compact_style
@already_warned_about_namespace_in_compact_style ||= Set.new
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if this approach is necessary. What I found is that I could get offenses to render correctly with nested modules but the auto-correct was clobbering the tree.

I think warning once on the outer namespace could probably get the point across.

@markburns markburns force-pushed the add-namespaced-class-and-module-children branch 4 times, most recently from e5db6b0 to f960082 Compare June 21, 2021 15:04
@markburns markburns marked this pull request as ready for review June 21, 2021 15:05
This adds support for a style wherein the concept of a module
used as a namespace is kept separate from the definition of a
class or module itself.

Since the introduction of Zeitwerk in Rails the benefits of
retaining the nested style in terms of accurate constant lookup have
disappeared.

This makes the compact style seem appealing for the following
reasons:

* Less low information value horizontal whitespace.
* Less git churn when moving files around between differing namespace depths.
* Easier scanning on the eyes when looking for the main constant.

The compact style suffers slightly from the following though:
* Main constant definition in a file is far over to the right in heavily namespaced files.
* Change to either the namespace or the constant name results in change to the same line.

The new proposed namespace style allows for
* Conceptual separation of the namespace/package concept from the class or
   module definition.
* File move refactors to a different namespace result in a diff that only touches
the namespace constant part of the file.
* Scanning by eye for the constant name is easier.

```ruby
module Foo
  module Bar
    module Baz
      module Qux
        module Quux
          module Quuz
            module Corge
              module Grault
                module Garply
                  module Waldo
                    module Fred
                      class Plugh
                      end
                    end
                  end
                end
              end
            end
          end
        end
      end
    end
  end
end
```

```ruby
module Foo::Bar::Baz::Qux::Quux::Quuz::Corge::Grault::Garply::Waldo::Fred
  class Plugh
  end
end
```

```ruby
class Foo::Bar::Baz::Qux::Quux::Quuz::Corge::Grault::Garply::Waldo::Fred::Plugh
end
```
@markburns markburns force-pushed the add-namespaced-class-and-module-children branch from f960082 to bd2f4d1 Compare June 21, 2021 15:42
@markburns markburns marked this pull request as draft June 24, 2021 05:44
@markburns
Copy link
Contributor Author

Converted back to draft as I found a couple of edge cases after running against a large project

@dvandersluis
Copy link
Member

@markburns ping :)

@markburns
Copy link
Contributor Author

sorry @dvandersluis just seen this - I need to get back on this at some point. IIRC I felt that it may make more sense in the rails gem as the only approach I found that resolved some ambiguity depended upon using the path and checking against zeitwerk conventions.

Whilst probably most projects do follow those conventions it felt a little prescriptive? Not sure how you'd feel about that?

I'll see if I can put some time into it this weekend

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.

None yet

2 participants