Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Fix semantic indentation of quoted functions#49

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
bbatsov merged 1 commit intoclojure-emacs:mainfromrschmukler:main
Nov 4, 2024

Conversation

@rschmukler
Copy link
Contributor

Fixes an error where quoted functions would not align correctly with semantic indentation. Adds an example to the test sample.

Before submitting a PR mark the checkboxes for the items you've done (if you
think a checkbox does not apply, then leave it unchecked):

  • You've runM-x checkdoc and fixed any warnings in the code you've written.
  • You've updated the changelog (if adding/changing user-visible functionality).
  • You've updated the readme (if adding/changing user-visible functionality).

Thanks!

"Return non-nil if NODE is a Clojure keyword."
(string-equal"kwd_lit" (treesit-node-type node)))

(defunclojure-ts--quoted-var-node-p (node)
Copy link
Member

Choose a reason for hiding this comment

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

I think the right terminology here isvar-quoted-node. That's because you can also have the standard quoting with'.

rschmukler reacted with thumbs up emoji
(string-equal"kwd_lit" (treesit-node-type node)))

(defunclojure-ts--quoted-var-node-p (node)
"Return non-nil if NODE is a Clojure quoted."
Copy link
Member

Choose a reason for hiding this comment

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

Clojure -> var-quoted

rschmukler reacted with thumbs up emoji
(clojure.core/filter even?
(range110))

(#'filter even?
Copy link
Member

Choose a reason for hiding this comment

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

I the indentation OK if it was just'filter? Also it might be good to change the example to make more sense in the context of quoting being used.

Copy link
ContributorAuthor

@rschmuklerrschmuklerJul 26, 2024•
edited
Loading

Choose a reason for hiding this comment

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

I am not sure how'filter should indent. Specifically, the reason I think a quotedvar should indent like this, is because it implements'IFn (ie. it can be called) while a symbol doesn't. Let me know if you agree with this. On the others, happy to change the name.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm OK with this.

@rschmukler
Copy link
ContributorAuthor

rschmukler commentedJul 26, 2024•
edited
Loading

@bbatsov Upon thinking about this further, I think that the proper name should actually bevar-node-p orvariable-node-p. The problem is that there is already avariable-node-p which returns whether a node is adef ordefonce. I think we should do the following:

  1. Rename the existingvariable-node-p tovariable-definition-node-p
  2. Renamequoted-var-node-p tovar-node-p

I think calling it anything else will be a bit confusing since it is technically a variable... ie:

(type #'filter);; => clojure.lang.Variable

I have updated the PR with these changes, but I am also happy to change it to whatever you want. Let me know!

bbatsov reacted with thumbs up emoji

@rschmukler
Copy link
ContributorAuthor

@bbatsov just pinging you on this. No rush, just a reminder :)

@bbatsov
Copy link
Member

@rschmukler Sorry about me dropping the ball on this earlier. You'll need to rebase onmaster and we can wrap it up.

@rschmukler
Copy link
ContributorAuthor

@bbatsov rebased and ready to go 👍

@bbatsov
Copy link
Member

Can you please address the small lint error that's causing the lint check to fail?

Also - now that we have some unit tests (see#57), you might want to add one covering your change.

@rschmukler
Copy link
ContributorAuthor

@bbatsov the PR is updated with listing passing. I tried to make the tests run and added (what I thought was an appropriate case toclojure-mode-indentation-test.el -however it looks like some of the tests in there aren't running? Specifically I was adding it to thedescribe "should handle function spec" section. Something like:

(when-indenting-it "should handle var based funcalls with style 'align-arguments"    'align-arguments    "(#'some-function  10  1  2)"    "(#'some-function 10                 1                 2)")

However when I runmake test I don't see any of the function related indentation tests running. Let me know what I am missing here.

@bbatsov
Copy link
Member

Those are the actual tests that we currently runhttps://github.com/clojure-emacs/clojure-ts-mode/blob/main/test/clojure-ts-mode-indentation-test.el

The ones in the other folder need to be ported to the new test suite.

@rschmukler
Copy link
ContributorAuthor

Got it! Thanks for the explanation and sorry for missing that. Updated the test suite to include a unit test for this.

@bbatsov
Copy link
Member

Hmm, seems that on Emacs snapshot there's a load error for the test file.

Fixes an error where quoted functions would not align correctly withsemantic indentation.Adds an example test and updates the changelog
@rschmukler
Copy link
ContributorAuthor

Sorry about that, should be good now

@bbatsovbbatsov merged commit3ca382c intoclojure-emacs:mainNov 4, 2024
3 checks passed
@bbatsov
Copy link
Member

Thanks!

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@bbatsovbbatsovbbatsov left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rschmukler@bbatsov
[8]ページ先頭
©2009-2025 Movatter.jp