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

Code folding #6

Merged
merged 10 commits into from
Apr 2, 2024
Merged

Code folding #6

merged 10 commits into from
Apr 2, 2024

Conversation

ramblehead
Copy link
Contributor

Some code folding fixes

see #5 for details

Should fold the following code correctly

```tsx
// Hey Emacs, this is -*- coding: utf-8 -*-

'use client';

import { Provider, defaultTheme } from '@adobe/react-spectrum';

interface Props {
  children: React.ReactNode;
}

export const App = ({ children }: Props): JSX.Element => {
  return (
    <Provider
      theme={defaultTheme}
      // locale={yourLocaleHere}
    >
      <main className="flex min-h-screen flex-col items-center justify-between p-24">
        {children}
      </main>
      <div>
        test
      </div>
    </Provider>
  );
};
```
Emacs 29 has forward-sexp (&optional arg interactive) arguments rather
than (forward-sexp n)

The calls to forward-sexp() should be compatible between Emacs
versions as long as INTERACTIVE argument is not used.
Should fold 'content' value correctly in the following snippet:

```javascript
module.exports = {
  important: '#app',

  content: [
    './src/pages/**/*.{js,ts,jsx,tsx,mdx}',
    './src/components/**/*.{js,ts,jsx,tsx,mdx}',
    './src/app/**/*.{js,ts,jsx,tsx,mdx}',
  ],

  presets: [spectrumPreset],

  plugins: [],
};
```
... and guarding against recursive call when forward-sexp-function
variable points to jtsx-hs-forward-sexp function
...in jtsx modes that handle JSX/TSX blocks and some minor cleanups
Limitations:

 * jtsx-backward-up-list() does not support backward-up-list arguments -
   always jumps one level up.

Also, need to investigate further if such function is required. It
might be possible to achive similar functionality with plain
backward-up-list() by refactoring jtsx-hs-forward-sexp() behaviour.
@llemaitre19
Copy link
Owner

Thanks, I will review it as soon I have some free time !

Pasting here the description of the PR done in #5.

  • The hide-show code is a blend of treesit and regexp approaches. It’s uncertain if this is the best practice. It might need to be refactored to rely solely on treesit queries.

  • hide-show does not fold js/ts code inside {} fragments in jsx blocks – it only folds the entire {} fragment.

  • The function jtsx-hs-forward-sexp does not fully support forward-sexp arguments - it always jumps one sexp forward for a positive argument and one sexp backward for a negative argument.

  • The function jtsx-hs-forward-sexp does not handle js/ts blocks inside jsx blocks.

  • The function jtsx-backward-up-list does not support backward-up-list arguments - it always jumps up one level.

Copy link
Owner

@llemaitre19 llemaitre19 left a comment

Choose a reason for hiding this comment

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

Just finished to review your PR.

Here the summary of what I did:

  • Test your changes -> OK for me !
  • Add 2 commits : minor changes in the documentation and some new tests
  • Leave 2 comments in the review

I have 2 more questions/remarks :

  • It appears that jtsx-hs-forward-sexp is not only intented for hideshow now, maybe we should rename it into jtsx-forward-sexp ?
  • jtsx-backward-up-list is independant of hideshow and is intended to be used as a replacement of backward-up-list for jtsx modes, if I am not wrong ? So we should document it as a public function in the README I guess (and warning about restrictions) ?

Concerning your concerns:

The hide-show code is a blend of treesit and regexp approaches. It’s uncertain if this is the best practice. It might need to be refactored to rely solely on treesit queries.

Yes but I don't think there any issues with that excepted in terms of performance.
For having a pure tree-sitter code folding, we should use something like ts-fold. However it is not on a package manager yet.

hide-show does not fold js/ts code inside {} fragments in jsx blocks – it only folds the entire {} fragment.

The function jtsx-hs-forward-sexp does not handle js/ts blocks inside jsx blocks.

js code nested in jsx is not straightforward to detect with tree-sitter. I worked on that point few weeks ago for the comment/uncomment function. But it is a quite hacky implementation. I think it would need to be rewritten to be easily used outside of comment/uncomment functionality.

The function jtsx-hs-forward-sexp does not fully support forward-sexp arguments - it always jumps one sexp forward for a positive argument and one sexp backward for a negative argument.

The function jtsx-backward-up-list does not support backward-up-list arguments - it always jumps up one level.

Ok. Anyway, it is a good start, and it could be enhanced later if necessary.

jtsx.el Show resolved Hide resolved
jtsx.el Show resolved Hide resolved
@llemaitre19 llemaitre19 merged commit 9bf55aa into llemaitre19:master Apr 2, 2024
@llemaitre19 llemaitre19 mentioned this pull request Apr 2, 2024
@ramblehead
Copy link
Contributor Author

Thank you for merging. Great work! In the upcoming iteration, I plan to delve into ts-fold to explore potential ideas we might integrate or even consider its use as a git submodule. I will also look into strategies to detect JS code nested within JSX, starting with the implementation of your commenting code. If anyone has insights or suggestions on these topics, please feel free to share!

@llemaitre19
Copy link
Owner

Glad to hear you are planning some others contributions.

I plan to delve into ts-fold to explore potential ideas we might integrate or even consider its use as a git submodule

I am just remembering that ts-fold did not support build-in tree-sitter 6 months ago, And sadly nothing seems to have changed since then... (issue #48)

If anyone has insights or suggestions on these topics, please feel free to share!

I have activated Discussions tab for that purpose.

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.

2 participants