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

Indentation points #1

Open
simonjwright opened this issue Oct 17, 2024 · 8 comments
Open

Indentation points #1

simonjwright opened this issue Oct 17, 2024 · 8 comments

Comments

@simonjwright
Copy link

gpr-mode indented like these:

   for Source_Dirs use Common.Paths &
     (
      "adainclude",
      "rp2350_svd",
      FreeRTOS.Source,
      FreeRTOS.Portable_Base & "RISC-V"
     );

   for Switches ("s-traceb.adb") use ALL_ADAFLAGS & ("-g")
      & NO_SIBLING_ADAFLAGS & ("-fno-inline-functions-called-once");

but gpr-ts-mode like these:

   for Source_Dirs use Common.Paths &
                         (
                          "adainclude",
                          "rp2350_svd",
                          FreeRTOS.Source,
                          FreeRTOS.Portable_Base & "RISC-V"
                         );

   for Switches ("s-traceb.adb") use ALL_ADAFLAGS & ("-g")
                                       & NO_SIBLING_ADAFLAGS & ("-fno-inline-functions-called-once");

It looks as though gpr-mode treats for-use statements rather like expressions, whereas gpr-ts-mode indents to the first token after use.

I don’t say there’s anything wrong, just something I’ll need to get used to .. well worth the effort, thank you!

Looking at the second example, does gpr-ts-mode have a line length (like the 79 in ada-mode) that it’ll automatically fold long lines to, or does it require the user to insert newlines where they want the folds?

@brownts
Copy link
Owner

brownts commented Oct 17, 2024

It looks as though gpr-mode treats for-use statements rather like expressions, whereas gpr-ts-mode indents to the first token after use.

Currently, gpr-ts-mode will try to keep the indentation of an expression together, that's why it aligns that way. I think I was trying to be clever, because it seemed to make sense to me that you'd want to keep the expression together, but the more I have thought about it, I wondered if that was really what people were expecting. I'd like the indentation to do what people think it should do. If you put the first part of the expression on the next line, it will align to that.

   for Source_Dirs use
     Common.Paths &
       (
        "adainclude",
        "rp2350_svd",
        FreeRTOS.Source,
        FreeRTOS.Portable_Base & "RISC-V"
       );

   for Switches ("s-traceb.adb") use
     ALL_ADAFLAGS & ("-g")
       & NO_SIBLING_ADAFLAGS & ("-fno-inline-functions-called-once");

I also have an additional indentation parameter (above what gpr-mode has), gpr-ts-mode-indent-exp-item-offset, which is used to indent subexpressions within a larger expression. You can see that behavior in the above based on how the parenthesis are indented relative to the start of the expression. I've recently thought that is probably not what people are expecting so I was planning to default this value to 0. If, so this ends up looking like this:

   for Source_Dirs use
     Common.Paths &
     (
      "adainclude",
      "rp2350_svd",
      FreeRTOS.Source,
      FreeRTOS.Portable_Base & "RISC-V"
     );

   for Switches ("s-traceb.adb") use
     ALL_ADAFLAGS & ("-g")
     & NO_SIBLING_ADAFLAGS & ("-fno-inline-functions-called-once");

I don’t say there’s anything wrong, just something I’ll need to get used to

I'm open to opinions on how you think this should behave. I'm working on indentation support for ada-ts-mode as well, and these types of decisions would flow into that too. I thought I was making an improvement over what was provided in gpr-mode, but if it's not what people expect, it might not be an improvement.

.. well worth the effort, thank you!

Thanks, I'm glad you find it useful!

Looking at the second example, does gpr-ts-mode have a line length (like the 79 in ada-mode) that it’ll automatically fold long lines to, or does it require the user to insert newlines where they want the folds?

Currently, there is nothing in the mode to support folding lines. I could look into that, but it hadn't crossed my mind.

@simonjwright
Copy link
Author

I think I’d keep gpr-ts-mode-indent-exp-item-offset, set to 2. It’s one of those things where sometimes, even within the same file, some cases feel better one way, some the other. The operative word being "feel".

Currently, there is nothing in the mode to support folding lines. I could look into that, but it hadn't crossed my mind.

Sometimes, with ada-mode, you’d type the final newline of a section and the whole section would re-format itself, folding included!

And, automatically choosing where the folds go feels rather overbearing. So I wouldn’t worry.

@brownts
Copy link
Owner

brownts commented Oct 21, 2024

Thanks for the input, I'll keep the default for gpr-ts-mode-indent-exp-item-offset.

While it doesn't address folding, I did release a new version of gpr-ts-mode yesterday that supports declaration-based indentation (now the new default). This will attempt to re-indent an entire declaration, if there are no syntax errors in the declaration. In the presence of syntax errors, it will fallback to the normal line-based indentation. The intention of this is to recover from previously incorrect indentation due to the presence of syntax errors. You can go back to the previous indentation strategy by changing gpr-ts-mode-indent-strategy to line.

I'm hoping this will be an improvement over the previous line-based indentation which would leave the buffer needing to be reformatted when multi-line constructs (i.e., case construction , package declaration, project declarations, etc.) were not syntactically correct when indentation was performed.

If you don't mind me asking, how did you enable/configure folding? I poked around at the gpr-mode code, but didn't see anything to configure, nor did I find documentation on it, but maybe I just wasn't looking in the correct location.

@simonjwright
Copy link
Author

The new strategy feels very comfortable.

I didn’t write the code for folding in gpr-mode (or ada-mode; the same engine, I believe). Long ago I wrote a formatter for Coral 66, which had concepts of single & parallel fold points (the parallel ones were for e.g. parameter lists, so that if one had to fold they all did). The engine laid out the output line from the left, laying exception handlers at the fold points; when it hit the end of the line, it raised an exception; the lowest handler would then convert itself to a fold (if parallel, ditto for all the siblings); start again at the beginning; repeat until the end of the input line is reached, then dump the line with the folds. I don’t remember how the indentation was managed, this was somewhere about 1995!

One point - it turns out that the SPARK tool gnatprove has its own GPR package Prove. gpr-ts-mode handles it OK, even though it’s not in gpr-ts-mode-package-names.

@brownts
Copy link
Owner

brownts commented Nov 10, 2024

The new strategy feels very comfortable.

I personally thought it dramatically improved the indentation situation, so glad to hear you find it works well too.

I didn’t write the code for folding in gpr-mode (or ada-mode; the same engine, I believe). Long ago I wrote a formatter for Coral 66, which had concepts of single & parallel fold points (the parallel ones were for e.g. parameter lists, so that if one had to fold they all did). The engine laid out the output line from the left, laying exception handlers at the fold points; when it hit the end of the line, it raised an exception; the lowest handler would then convert itself to a fold (if parallel, ditto for all the siblings); start again at the beginning; repeat until the end of the input line is reached, then dump the line with the folds. I don’t remember how the indentation was managed, this was somewhere about 1995!

That's quite interesting. I had to look up Coral 66 as I had never heard of it before. Sounds like it was at least somewhat inspirational for what became Ada. I feel like the wrapping engine you describe is partially how the indentation engine works (from a top-down perspective). I do wonder if the wrapping part might be covered by the Emacs auto-fill-mode.

I am curious if the following might get you close to what you're expecting. I thought that maybe ada-mode might have enabled auto-fill-mode by default, but when I started it up, I didn't notice that it did. The following is something you could try in a GPR buffer to see if this meets your expectation.

M-: (setq-local fill-indent-according-to-mode t)
M-: (setq-local fill-column 80)
M-x auto-fill-mode

One point - it turns out that the SPARK tool gnatprove has its own GPR package Prove. gpr-ts-mode handles it OK, even though it’s not in gpr-ts-mode-package-names.

Thanks, I'll add it to the list in the next release. That variable is currently only used to adjust the syntax highlighting for variable/attribute/package references. For example something like "A.B.C'Attribute", it's ambiguous if "C" is a reference to a package in project "A.B" or if it is a reference to a child project. Since package names get special syntax highlighting, gpr-ts-mode-package-names is used to resolve the ambiguity in the reference.

As a side note, I'm working on improving the indentation in the presence of syntax errors. It tries to match the nearest "case", "package", "project", etc. in order to anchor the indentation, when there are syntax errors. So far, it works quite well when writing a GPR from a top-down perspective.

Additionally, I'm working on some completion functionality. Currently I have package/project names after "end" (which completes for the name at the beginning of the project/package) as well as the name for packages (which pulls it's suggestions from gpr-ts-mode-package-names). I'm not sure if you use the LSP to provide completions for your GPR files, but the current version doesn't seem to provide completions after "end". Therefore, I've added this in as it was annoying me.

Just as quick demonstration, the following shows the behavior in my current work-in-progress updates (not yet officially available):

output-2024-11-09-21:10:46

@brownts
Copy link
Owner

brownts commented Dec 6, 2024

Just FYI, there is a new release which includes the above demonstrated functionality. In addition I've added casing commands and auto-casing support. Auto-casing is implemented as a minor mode (gpr-ts-auto-case-mode).

@simonjwright
Copy link
Author

Indeed! I’ve only needed tiny GPRs so far, but looking very good. I have yet to get used to the intellisense feature, and the behaviour when attempting to put parens or quotes round existing text (e.g. with point at f in foo, typing ( results in ()foo); but, getting there!

A couple of minor things:


How should gpr-ts-auto-case-mode be automatically invoked? I’ve tried

(use-package gpr-ts-mode
  :defines (org-src-lang-modes)
  :custom (gpr-ts-mode-grammar-install 'auto)
  :init
  (with-eval-after-load 'org-src
    (add-to-list 'org-src-lang-modes '("gpr" . gpr-ts)))
  (add-hook
   'gpr-ts-mode-hook
   (lambda()
     (gpr-ts-auto-case-mode)                            ;; <<<<<
     (setq-local comment-padding "  ")
     (add-hook `before-save-hook `delete-trailing-whitespace nil t)
     (add-hook 'before-save-hook
               (lambda () (untabify (point-min) (point-max))) nil t)     
     )))

but it doesn’t have any effect.


I get this:

image

That is the recommended form.

@brownts
Copy link
Owner

brownts commented Dec 9, 2024

I suspect you're running with the GPR LS enabled and that is providing some of this. For example, I'm not quite sure the Intellisense feature you're referring to, but it very well could be the completions provided by the language server being presented via company-mode.

As a note, I'd like to point out some performance issues with the language server, as documented in this issue.

I have yet to get used to the intellisense feature, and the behaviour when attempting to put parens or quotes round existing text (e.g. with point at f in foo, typing ( results in ()foo); but, getting there!

This sounds like you have electric-pair-mode or electric-pair-local-mode enabled. That's not required to use gpr-ts-mode, so you can disable it in your configuration if you don't like that behavior. I had originally suggested it before in the example configuration to help keep the syntax closer to a valid state, but that really shouldn't be necessary with all of the new support for indentation in the presence of invalid syntax.

How should gpr-ts-auto-case-mode be automatically invoked? I’ve tried

(use-package gpr-ts-mode
  :defines (org-src-lang-modes)
  :custom (gpr-ts-mode-grammar-install 'auto)
  :init
  (with-eval-after-load 'org-src
    (add-to-list 'org-src-lang-modes '("gpr" . gpr-ts)))
  (add-hook
   'gpr-ts-mode-hook
   (lambda()
     (gpr-ts-auto-case-mode)                            ;; <<<<<
     (setq-local comment-padding "  ")
     (add-hook `before-save-hook `delete-trailing-whitespace nil t)
     (add-hook 'before-save-hook
               (lambda () (untabify (point-min) (point-max))) nil t)     
     )))

but it doesn’t have any effect.

That looks fine to me. That configuration worked and enables auto-casing when I drop it into my configuration. Does this work when you enable it manually? (M-x gpr-ts-auto-case-mode). If not, I suspect something else might be amiss here.

I get this:

image That is the recommended form.

That's a diagnostic coming from the GPR LS (and you should see that in VSCode as well). It looks like this might have been fixed recently in commit AdaCore/ada_language_server@bbe2f54

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

2 participants