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 clj-kondo support for cljd.flutter.alpha/widget #29

Merged
merged 2 commits into from
Apr 21, 2022

Conversation

D00mch
Copy link
Contributor

@D00mch D00mch commented Apr 19, 2022

I decided to use /hooks as a directory inside clj-kondo.exports/<your-org>/<your-libname>/
somewhat like sicmutils does.

Why is .git ignore changed?

Without it there would be generated files:
image

It doesn't depend on my implementation, I checked better-cond, there is the same problem after opening it's files with lsp running:
image

Where does it started?

https://clojurians.slack.com/archives/C03A6GE8D32/p1650354879575349

Links to check

(:require [clj-kondo.hooks-api :as api]))

(defn widget [{:keys [:node]}]
(let [[state-key binding-vec & body] (rest (:children node))
Copy link
Contributor Author

@D00mch D00mch Apr 19, 2022

Choose a reason for hiding this comment

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

todo: also support :key, :watch and :context done

@D00mch
Copy link
Contributor Author

D00mch commented Apr 19, 2022

Still working on it... done

(seq unknown-keys)
(error (str "Unknown keys " (str/join " " unknown-keys)))

(not= 1 (- (count args) bindings-count)) ;; body is the one that remains
Copy link
Contributor

Choose a reason for hiding this comment

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

body length can be anything, even if it will be most of the time 1, it may be more in case of side-effects, a length of 0 would be most certainly an error.

Copy link
Contributor Author

@D00mch D00mch Apr 20, 2022

Choose a reason for hiding this comment

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

I guess, something like this will do the job:

:else 
        {:node (api/list-node 
                 (list
                   (api/token-node 'let)
                   (api/vector-node (:children state))
                   watch
                   key
                   body))}

It generates something like (let [state-name state-value] watch-value, key-value, body)

Only context allow to generate new symbols
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

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 will see what else could be done, today evening

(error (str "Unknown values"))

:else
(let [check-symbols #{:watch :key}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what's this function must return. There seems to be keywords in binding position in the generated let.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it returns something like:

(let [context-name-user-provided identity ; if user privides context
      state-name state-value ; if user provides a state 
      ]

    ;; need it to check if these symbols exist 
    watch ; could be nil
    key ; could be nil 

    (widget. (body. (creation. )))

@D00mch D00mch force-pushed the main branch 2 times, most recently from 33b6b39 to f19217c Compare April 20, 2022 16:49
Copy link
Contributor

@cgrand cgrand left a comment

Choose a reason for hiding this comment

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

If you could add the symbol check on state name, merge all state checks in one single branch that would be perfect and I would merge.
I also provided variants to your messages.

Thanks!

#{:state :key :watch :context})]

(cond
(and (seq (filter api/keyword-node? args)) ; has top keywords
Copy link
Contributor

Choose a reason for hiding this comment

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

could raise some false positive but it's so unlikely and useless that's ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What examples do you think may become a false positive?
Only when there is a key-word pair, it raises an exception
image

Comment on lines 32 to 36
(and state (not (api/vector-node? state)))
(error "State should be a vector [name initial-value]")

(and state (not= (count (:children state)) 2))
(error "State should contain only one name-value pair")
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe these two branches should be merged and the error message should read ":state should be a vector [name initial-value]". Btw you should also check in that branch that name is a simple symbol.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will merge them.

As for symbol name — it's already checked by the let form:
image

(error "State should contain only one name-value pair")

(seq unknown-keys)
(error (str "Unknown keys " (str/join " " unknown-keys)))
Copy link
Contributor

Choose a reason for hiding this comment

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

"Unsupported option keys: "

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would look like
image

So may be remove ':' after keys? Or its fine?

(error (str "Unknown first form " (api/sexpr (first args))))

(and state watch)
(error "State and watch keys are mutually exclusive")
Copy link
Contributor

Choose a reason for hiding this comment

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

":state and :watch option keys are mutually exclusive."

@cgrand
Copy link
Contributor

cgrand commented Apr 21, 2022 via email

@D00mch
Copy link
Contributor Author

D00mch commented Apr 21, 2022

I see, didn't think about destructuring. I will fix it too.

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