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

Poly command fails when source has non-seq import items #73

Closed
futuro opened this issue Mar 11, 2021 · 6 comments
Closed

Poly command fails when source has non-seq import items #73

futuro opened this issue Mar 11, 2021 · 6 comments
Assignees

Comments

@futuro
Copy link
Contributor

futuro commented Mar 11, 2021

Describe the bug
Having an import that isn't in a seq causes polylith cli to throw an exception. If you wrap your import in a vector or list, poly works as expected. This is also true for requires.

To Reproduce
Steps to reproduce the behavior:

  1. Have a namespace with a bare import, like so
(ns example.ns
  (:import java.time.LocalDateTime))
  1. Run poly info
  2. See error Don't know how to create ISeq from: clojure.lang.Symbol

Expected behavior
To see the poly info output, and not an exception string.

Screenshots
N/A

Workspace Attachment
I'm pretty sure this isn't applicable, but let me know if you need it.

Operating System (please complete the following information):

  • OS: Arch Linux
  • Version: Latest

Versions (please complete the following information):

  • Java: 15.0.2
  • Poly: Latest release, as well as hash ec38a83db7597e6f6f33dfbc1e4da665807ab091

Additional context

Stacktrace:

java.lang.IllegalArgumentException: Don't know how to create ISeq from: clojure.lang.Symbol
 at clojure.lang.RT.seqFrom (RT.java:557)
    clojure.lang.RT.seq (RT.java:537)
    clojure.lang.RT.first (RT.java:693)
    clojure.core$first__5402.invokeStatic (core.clj:55)
    clojure.core/first (core.clj:55)
    polylith.clj.core.workspace_clj.namespaces_from_disk$import$fn__6076.invoke (namespaces_from_disk.clj:15)
    clojure.core$map$fn__5885.invoke (core.clj:2759)
    clojure.lang.LazySeq.sval (LazySeq.java:42)
    clojure.lang.LazySeq.seq (LazySeq.java:58)
    clojure.lang.Cons.next (Cons.java:39)
    clojure.lang.RT.length (RT.java:1785)
    clojure.lang.RT.seqToArray (RT.java:1726)
    clojure.lang.LazySeq.toArray (LazySeq.java:132)
    clojure.lang.RT.toArray (RT.java:1699)
    clojure.core$to_array.invokeStatic (core.clj:346)
    clojure.core$sort.invokeStatic (core.clj:3101)
    clojure.core$sort.invokeStatic (core.clj:3090)
    clojure.core$sort.invoke (core.clj:3090)
    polylith.clj.core.workspace_clj.namespaces_from_disk$imports.invokeStatic (namespaces_from_disk.clj:18)
    polylith.clj.core.workspace_clj.namespaces_from_disk$imports.invoke (namespaces_from_disk.clj:17)
    polylith.clj.core.workspace_clj.namespaces_from_disk$__GT_namespace.invokeStatic (namespaces_from_disk.clj:34)
    polylith.clj.core.workspace_clj.namespaces_from_disk$__GT_namespace.invoke (namespaces_from_disk.clj:29)
    polylith.clj.core.workspace_clj.namespaces_from_disk$namespaces_from_disk$fn__6084.invoke (namespaces_from_disk.clj:37)
    clojure.core$mapv$fn__8470.invoke (core.clj:6917)
    clojure.lang.PersistentVector.reduce (PersistentVector.java:343)
    clojure.core$reduce.invokeStatic (core.clj:6832)
    clojure.core$mapv.invokeStatic (core.clj:6908)
    clojure.core$mapv.invoke (core.clj:6908)
    polylith.clj.core.workspace_clj.namespaces_from_disk$namespaces_from_disk.invokeStatic (namespaces_from_disk.clj:37)
    polylith.clj.core.workspace_clj.namespaces_from_disk$namespaces_from_disk.invoke (namespaces_from_disk.clj:36)
    polylith.clj.core.workspace_clj.components_from_disk$read_component.invokeStatic (components_from_disk.clj:14)
    polylith.clj.core.workspace_clj.components_from_disk$read_component.invoke (components_from_disk.clj:8)
    polylith.clj.core.workspace_clj.components_from_disk$read_components$fn__6199.invoke (components_from_disk.clj:26)
    clojure.core$map$fn__5885.invoke (core.clj:2759)
    clojure.lang.LazySeq.sval (LazySeq.java:42)
    clojure.lang.LazySeq.seq (LazySeq.java:51)
    clojure.lang.RT.seq (RT.java:535)
    clojure.core$seq__5420.invokeStatic (core.clj:139)
    clojure.core$sort.invokeStatic (core.clj:3101)
    clojure.core$sort_by.invokeStatic (core.clj:3107)
    clojure.core$sort_by.invokeStatic (core.clj:3107)
    clojure.core$sort_by.invoke (core.clj:3107)
    polylith.clj.core.workspace_clj.components_from_disk$read_components.invokeStatic (components_from_disk.clj:26)
    polylith.clj.core.workspace_clj.components_from_disk$read_components.invoke (components_from_disk.clj:25)
    polylith.clj.core.workspace_clj.core$workspace_from_disk.invokeStatic (core.clj:50)
    polylith.clj.core.workspace_clj.core$workspace_from_disk.invoke (core.clj:30)
    polylith.clj.core.workspace_clj.core$workspace_from_disk.invokeStatic (core.clj:38)
    polylith.clj.core.workspace_clj.core$workspace_from_disk.invoke (core.clj:30)
    polylith.clj.core.workspace_clj.interface$workspace_from_disk.invokeStatic (interface.clj:7)
    polylith.clj.core.workspace_clj.interface$workspace_from_disk.invoke (interface.clj:4)
    polylith.clj.core.command.core$read_workspace.invokeStatic (core.clj:56)
    polylith.clj.core.command.core$read_workspace.invoke (core.clj:50)
    polylith.clj.core.command.core$execute.invokeStatic (core.clj:66)
    polylith.clj.core.command.core$execute.invoke (core.clj:62)
    polylith.clj.core.command.interface$execute_command.invokeStatic (interface.clj:5)
    polylith.clj.core.command.interface$execute_command.invoke (interface.clj:4)
    polylith.clj.core.poly_cli.core$_main.invokeStatic (core.clj:30)
    polylith.clj.core.poly_cli.core$_main.doInvoke (core.clj:7)
    clojure.lang.RestFn.applyTo (RestFn.java:137)
    clojure.lang.Var.applyTo (Var.java:705)
    clojure.core$apply.invokeStatic (core.clj:667)
    clojure.main$main_opt.invokeStatic (main.clj:514)
    clojure.main$main_opt.invoke (main.clj:510)
    clojure.main$main.invokeStatic (main.clj:664)
    clojure.main$main.doInvoke (main.clj:616)
    clojure.lang.RestFn.applyTo (RestFn.java:137)
    clojure.lang.Var.applyTo (Var.java:705)
    clojure.main.main (main.java:40)
@furkan3ayraktar
Copy link
Collaborator

Hi @futuro

Thanks for taking time to open an issue. I have explained the rationale behind this behaviour in this issue. Using sequences in import and require statements is a best practice explained in detail here.

Although, I agree that the error message should be more descriptive and there should be a section in the documentation explaining this.

@furkan3ayraktar furkan3ayraktar self-assigned this Mar 11, 2021
@futuro
Copy link
Contributor Author

futuro commented Mar 12, 2021

Hi @furkan3ayraktar :)

Thanks for taking the time to respond. I read your response in the other issue, and it sounds like the impetus for the current behavior is because supporting the same forms of libspecs/import forms as clojure.core is non-trivial; is that an accurate read?

Relatedly, if I put together a PR that could handle all valid forms to define requires and imports, is that in line with the project goals/would you be interested in that?

How about a PR to detect forms that don't follow the how-to-ns suggestions and throw a more informative error?


Semi-related, after a little testing, I've discovered the current code silently does the wrong thing if you use namespace prefixes and the leaf-nodes point to different libraries. For example, if you have the following map in your deps.edn

:ns-to-lib {com.a    library-a
            com.a.b  library-b
            com.a.c  library-c}

And the following require form

(ns ,,,
  (:require [com.a b c]))

poly libs will report that library-a is being used instead of library-b and c.

I'm not sure exactly how prevalent this is, but it came to mind after I was re-reading the how-to-ns post and contemplating how I would parse the ns forms accepted by clojure.core.

@tengstrand
Copy link
Collaborator

Hi there!

Thanks for pointing out this problem.

The :ns-to-lib key will be removed in the next release when issue 66 is solved and then this bug will no longer exist.

Each brick will have its own deps.edn that specifies the libraries it uses, and projects will specify the bricks they use as dependencies by using the :local/root syntax instead of giving the paths. A migration tool will also be available so that existing projects can easily be migrated to the new format.

The poly tool will at that point support visualising both the old (current) and the new project format
so that you can checkout an old version of the codebase and still use commands like 'info' and 'libs'.

Hope that answered your question.

@futuro
Copy link
Contributor Author

futuro commented Mar 15, 2021

That's good to know about the :ns-to-lib changes.

Will that change also mean that poly isn't reading namespaces from disk anymore, or will it continue to do so?

@tengstrand
Copy link
Collaborator

I will continue reading namespaces from disk.
My plan is that it should read the metadata also
(so that doc strings and other metadata will be included in the "ws" output)
but I'm not sure when that will be implemented.

@tengstrand
Copy link
Collaborator

I close this issue now, since #74 is merged.

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

No branches or pull requests

3 participants