fabiodomingues / clj-depend

A Clojure namespace dependency analyzer
Eclipse Public License 2.0
78 stars 8 forks source link

If a layer is allowed by :accessed-by-layers or :accesses-layers, it should not throw a violation #43

Open thayannevls opened 1 year ago

thayannevls commented 1 year ago

Hello! I'm developing two configurations using clj-depend, where I want one configuration know about layers of the another but not the other way around.

Imagine that this is configuration A:

:clj-depend {:layers {:foo {:defined-by "foo" :accessed-by-layers #{:one :two}}
                       ...}}

And this is configuration B:

:clj-depend {:layers {:bar {:defined-by "bar" :accesses-layers #{:foo}}
                       ...}}

I don't want that config A knows about B, so it's not a option for me to add :bar in accessed-by-layers in :foo.

The thing is clj-depend throws a violation even though I defined :accesses-layers #{:foo}. I believe this happens because of this code:

(defn- violate?
  [config
   {:keys [layer dependency-layer]}]
  (and (not= layer dependency-layer)
       (or (dependency-layer-cannot-be-accessed-by-layer? config dependency-layer layer)
           (layer-cannot-access-dependency-layer? config layer dependency-layer))))

If you agree that it should not throw an error in this case, we could change to return true only if it's not accessible in any way:

(defn- violate?
  [config
   {:keys [layer dependency-layer]}]
  (and (not= layer dependency-layer)
       (and (dependency-layer-cannot-be-accessed-by-layer? config dependency-layer layer)
            (layer-cannot-access-dependency-layer? config layer dependency-layer))))
fabiodomingues commented 1 year ago

I was analyzing your case, and I even wrote a test scenario to simulate this.

(deftest analyze-test-using-access-layers-and-accessed-by-layers
  (testing "should return zero violations when there is a layer defined that is not accessed by any other but there is a layer that defines which accesses it"
    (is (= []
           (analyzer/analyze {:config                    {:layers {:a {:defined-by         ".*\\.a\\..*"
                                                                       :accessed-by-layers #{}}
                                                                   :b {:defined-by         ".*\\.b\\..*"
                                                                       :accessed-by-layers #{:a}}
                                                                   :c {:defined-by         ".*\\.c\\..*"
                                                                       :access-layers      #{:b}}}}
                              :dependencies-by-namespace {'foo.a.bar #{'foo.c.bar 'foo.b.bar}
                                                          'foo.b.bar #{'foo.b.baz}
                                                          'foo.b.baz #{}
                                                          'foo.c.bar #{'foo.b.bar}}})))))

BTW, when writing the test scenario it seemed contradictory to have a config that defines that a layer should not be accessed by any other when in the same config there is another layer defined that can access it.

Mixing :accessed-by-layers and :access-layers can lead to confusion. It seems to make sense for clj-depend to validate when this occurs and alert the user to only use one form. Wdyt?