seddonym / import-linter

Import Linter allows you to define and enforce rules for the internal and external imports within your Python project.
https://import-linter.readthedocs.io/
BSD 2-Clause "Simplified" License
666 stars 45 forks source link

Add support for JSON configuration files #177

Closed sydneypemberton1986 closed 1 year ago

sydneypemberton1986 commented 1 year ago

I also tested that the files are loaded correctly with the following manual test that I didn't think was appropriate to commit.

If there is a better way to test this, I'm happy to add the documentation to the contribution instructions.

diff --git .importlinter.json .importlinter.json
new file mode 100644
index 0000000..f1b3922
--- /dev/null
+++ .importlinter.json
@@ -0,0 +1,22 @@
+{
+    "root_package": "importlinter",
+    "contracts": [
+        {
+            "name": "Layered architecture",
+            "type": "layers",
+            "containers": [
+                "importlinter"
+            ],
+            "layers": [
+                "cli",
+                "api",
+                "contracts",
+                "configuration",
+                "adapters",
+                "application",
+                "domain"
+            ],
+            "exhaustive": true
+        }
+    ]
+}
diff --git .importlinter.toml .importlinter.toml
new file mode 100644
index 0000000..a97d406
--- /dev/null
+++ .importlinter.toml
@@ -0,0 +1,19 @@
+[tool.importlinter]
+root_package = "importlinter"
+
+[[tool.importlinter.contracts]]
+name="Layered architecture"
+type="layers"
+containers = [
+    "importlinter"
+]
+layers=[
+    "cli",
+    "api",
+    "contracts",
+    "configuration",
+    "adapters",
+    "application",
+    "domain"
+]
+exhaustive = true
\ No newline at end of file
diff --git tox.ini tox.ini
index cefd106..993dd50 100644
--- tox.ini
+++ tox.ini
@@ -30,6 +30,8 @@ commands =

 [testenv:check]
+allowlist_externals =
+    which
 deps =
     {[testenv]deps}
     black~=22.3.0
@@ -40,7 +42,10 @@ commands =
     black --check src tests
     flake8 src tests
     mypy src/importlinter tests
-    lint-imports
+    which lint-imports
+    lint-imports --config .importlinter.toml
+    lint-imports --verbose --config .importlinter.json
+

 [testenv:docs]
 deps =
sydneypemberton1986 commented 1 year ago

Hello @seddonym ! I wrote you a while back about the project we were planning for our company's modularity linter. The first step is upon is, which is adding support for JSON files. We need that support because the contracts we are looking to add will need to contain more complex structure than is supported by TOML such as nested objects.

It is our sincere hope that this will be accepted into the mainline package so we don't have to maintain a forked version.

seddonym commented 1 year ago

Hi Sydney, thanks for the PR!

I have to admit that I'm reticent to add support for JSON files at this point, for these reasons:

  1. I think it would be better if a contract could be expressed in any of the supplied formats. I think it would complicate the library if some contracts can only be expressed in certain formats.
  2. I'm not convinced that we couldn't express what you need with the INI or TOML files. For example you might be able to use whitespace or other ways of encoding things. Perhaps you could give a minimal example of something that you feel couldn't be expressed in the other formats?
  3. There's a cost to supporting extra formats.

Sorry if this is disappointing - I do recommend reaching out before starting the work on a PR to see if I'm broadly bought into the idea, as I hate to waste your time!

Did you have a chance to consider my reply to your original issue?

Final thought: if you absolutely must have JSON support you could probably do it without forking Import Linter, and instead just copying importlinter.cli and importlinter.configuration as different files. The configuration file could then pass in your custom UserOptionReader and importlinter.cli could import that configuration file instead.

Happy to keep discussing, let me know what you think.

sydneypemberton1986 commented 1 year ago

Hey David,

Thanks for your response. I am disappointed for sure, but I totally respect your reasons and your protectiveness of the consistency of the user experience, as well as your experience as a maintainer.

Here are some counterpoints, for what it's worth:

I'm not sure I understand your point about copying the files, but happy to look into that.

I did get a chance to read over your response and I very much appreciated it. I apologize for not responding there sooner.

Our situation is complex, and we do need to be able to group things in the way described. It gets us from "pie in the sky" to "implementable".

If we used the ignore mechanism, we would be ignoring thousands of imports. What we are going for is more a "modularity metric".

Thank you so much again for your wonderful project. And I just want to say I've enjoyed reading your code and seeing the hexagonal architecture at work.

Cheers, Sydney

sydneypemberton1986 commented 1 year ago

So I'm embarrassed to say to say that I was ignorant of TOML's dotted properties, which will work for us. Closing the PR.

seddonym commented 1 year ago

So I'm embarrassed to say to say that I was ignorant of TOML's dotted properties, which will work for us.

Oh, glad to hear!

I'm not sure I understand your point about copying the files, but happy to look into that.

I mean that there is a way you could enable JSON support while still working (mainly) on the released version of Import Linter. It's a bit awkard but you'd only need to fork a couple of files.

What we are going for is more a "modularity metric".

This is interesting. If that's the case, I wonder if Import Linter is the best fit: you could consider using my lower-level library Grimp instead, which is what Import Linter uses. You could then express what you need (in JSON if you want) and it could output the metric.

I'd be happy to have a chat at some point to better understand how you're using the tool, feel free to email me if you would like to arrange.