> On Nov 17, 2022, at 10:53 AM, João Paulo Labegalini de Carvalho
> <jaopaulolc@gmail.com> wrote:
>
>
> 1. I added commit message for your patch as an example. For future patches
> itâs best if you can also have this ChangeLog style commit message, I think
> you can find more detailed explanation in in CONTRIBUTE file.
>
> (I donât know if you know the following already, bug FYI:)
>
> 2. Docstring sentences always end with a period
> 3. All comments and sentences should be complete sentences, with two spaces
> at the end.
> 4. The first line of a docstring shouldnât exceeds 80 columns.
>
> I did not. Thanks for the corrections and improvements. I will keep those in
> mind for future patches.
>
> For the second commit, I changed all the feature names to singular, and
> decl-commands to declaration-command. I also simplified the rule for
> declaration-command, IIUC you want to highlight the command keywords?
>
> The changes to singular make sense to me. I considered your simplified
> version as well, but decided against alternation queries to avoid hardcoded
> command names. Matching a node via its type rather than comparing the
> spelling might be fast, but my code also had a string= there, so in the end
> the simpler version works well.
It should be safe to hardcode these command keywords, because they are
hardcoded in the grammar anyway (and I donât see them used elsewhere):
declaration_command: $ => prec.left(seq(
choice('declare', 'typeset', 'export', 'readonly', 'local'),
repeat(choice(
$._literal,
$._simple_variable_name,
$.variable_assignment
))
)),
>
> Also, right now these command keywords are highlighted in builtin face,
> should they be fontified in keyword face? Iâm no expert of bash so I canât
> tell. But keyword face seems more natural to me.
>
> You are correct in the sense that all declaration commands are builtin
> commands. I decided to highlight them differently than other builtin commands
> because they are used to define variables, and it might be useful to be able
> to distinguish them visually. But this is not required nor more correct than
> using the builtin face for both types of commands.
I meant to highlight them all in keyword face, but if you donât have objections
Iâll make that change :-) Thanks!
Yuan
RetroSearch is an open source project built by @garambo | Open a GitHub Issue
Search and Browse the WWW like it's 1997 | Search results from DuckDuckGo
HTML:
3.2
| Encoding:
UTF-8
| Version:
0.7.4