search

tajmone / ST4-Asciidoctor

AsciiDoc Package for SublimeText 4
https://tajmone.github.io/ST4-Asciidoctor
MIT License
11 stars 6 forks source link

Now supports Goto Definition/Goto reference/Goto Symbol between block IDs (anchors/links). Also, tweaked the scope names for headings. #47

Closed polyglot-jones closed 6 months ago

polyglot-jones commented 6 months ago

  1. Changed the block ID (anchor) scope from "markup.underline.blockid.id" to "entity.name.label" (per the syntax naming guidelines). Added a preference P-list for the block ID (entity.name.label) to be indexed and thus appear in the symbol list (for declaration goto targeting). Likewise, added a preference P-list for the block id within a link (variable.parameter.xref) to be indexed and thus appear in the symbol list (for reference goto targeting).

  2. Removed the extraneous "level" from "markup.heading.level.1.asciidoc", "markup.heading.level.2.asciidoc", etc. to align with what the color schemes expect (e.g. the Monokai and Breakers schemes set one color for all "markup.heading" and a second color for "markup.heading.1" in particular, and Celeste also sets a third color for "markup.heading.2").

tajmone commented 6 months ago

I've checkout your PR branch locally because I want to test thoroughly the changes before merging it.

In the future, all PRs will go through a dedicated dev branch, because I think that any commits that go into master lead to automatic update of the package for those users who installed via its Git repository URL — initially I thought that only tagged releases would be treated as updates, just like ordinary packages through the official channel, but now I've started to think that with Git-installed packages ST simply attempts to pull the latest main branch from the package repository. If that's the case, we should keep all new commits and contributions on the dev branch, and only merge them into master once we're sure that everything is working as expected.

Removed the extraneous "level" from "markup.heading.level.1.asciidoc", "markup.heading.level.2.asciidoc", etc. to align with what the color schemes expect (e.g. the Monokai and Breakers schemes set one color for all "markup.heading" and a second color for "markup.heading.1" in particular, and Celeste also sets a third color for "markup.heading.2").

I'm not really convinced about these changes. Bear in mind that markup syntaxes are unlikely to fall under the official ST guidelines, since these are designed with "classic languages" in mind only — e.g. try to create a BNF syntax following the official scoping guidelines, it's simply not possible, even though BNF are the foundation of every language that exists — as a matter of fact, there are several BNF/EBNF/ABNF packages for ST (and other editors which adopt TextMate-like scoping) and each adopts different scoping conventions.

According to the official ST scoping guidelines, headings should be scoped as entity.name.section — but no decent lightweight markup syntax package sticks to those. As a matter of fact, most good packages tend to peek into packages for other editors (VSCode, Atom, etc.) and see which scoping names they adopted, or consult online scoping references such as:

Compatibility with native color schemes shouldn't really be a concern in our case, because the AsciiDoc syntax is too different from the average programming languages, or even most markup syntaxes for that matter. IMO we should rather stick to good naming conventions that provide meaningful semantics, rather than stick to the rather poor official guidelines or comply with color schemes — which is why I chose to enforce the package color scheme in the first places.

If you look at the excellent Markdown package for ST (probably one of the most used and successful packages of ST) you'll see that it follows its own scoping conventions.

In our case, I think that markup.heading.level provides better semantics, because markup.heading should (at least in theory) be the entire scope of a section title, whereas in our case we're trying to scope just the = delimiter, which marks the heading level — in practice, though, in order to achieve Symbol indexing and other features (e.g. Goto Symbol) we're forced to stick with the few scopes that support such features, even if they are semantically at odds with the context being added — so much for the "semantic scoping guidelines" promoted by ST official documentation.

One of the reasons why I stopped publishing packages on the official channel (although not the only one) was because I was tired of having to deal with their tweak requests and debate over the official guidelines and whether they make sense (i.e. beyond C++, Python, and the usual languages). From what I've seen, most good packages are deviating from such guidelines, because in order to achieve good editing functionality (e.g. via plugins and other custom scripts) you need to adopt scoping semantics with suit the need of the syntax at hand, rather than the vision of ST creators and maintainers.

Anyhow, I'll keep testing your changes locally — just give me some time. As a general rule, I'd rather prefer that we discuss changes that revert the original design choices in the package, before submitting a PR containing such changes — usually that's how collaborative efforts work, through discussion and team planning.

P.S. — Please, avoid spurious commits in PRs — if you need to fix something, simply amend the original commit (or squash the multiple commits) and force push, the PR remains good.

tajmone commented 6 months ago

Merged in dev Branch Instead!

In the future, all PRs will go through a dedicated dev branch, because I think that any commits that go into master lead to automatic update of the package for those users who installed via its Git repository URL — initially I thought that only tagged releases would be treated as updates, just like ordinary packages through the official channel, but now I've started to think that with Git-installed packages ST simply attempts to pull the latest main branch from the package repository. If that's the case, we should keep all new commits and contributions on the dev branch, and only merge them into master once we're sure that everything is working as expected.

Therefore, instead of merging your pull request I've integrated it (manually) directly into the new dev branch — which from now will be the baseline branch for all contributions and PRs.

I also took the liberty of squashing the two commits into a single one, to preserve a clean and linear history, and revised the commit title, which was to long — don't use wording like "Now uses ...", the convention is to adopt a succinct imperative form, the goal being to provide easy to read and compact information.

P.S. — Please, avoid spurious commits in PRs — if you need to fix something, simply amend the original commit (or squash the multiple commits) and then force push, the PR remains good.

Thoughts on Scoping Conventions

Removed the extraneous "level" from "markup.heading.level.1.asciidoc", "markup.heading.level.2.asciidoc", etc. to align with what the color schemes expect (e.g. the Monokai and Breakers schemes set one color for all "markup.heading" and a second color for "markup.heading.1" in particular, and Celeste also sets a third color for "markup.heading.2").

Initially I wasn't really convinced about these changes. Bear in mind that markup syntaxes are unlikely to fall under the official ST guidelines, since these are designed with "classic languages" in mind only — e.g. try to create a BNF syntax following the official scoping guidelines, it's simply not possible, even though BNF are the foundation of every language that exists — as a matter of fact, there are several BNF/EBNF/ABNF packages for ST (and other editors which adopt TextMate-like scoping) and each adopts different scoping conventions.

According to the official ST scoping guidelines, headings should be scoped as entity.name.section — but no decent lightweight markup syntax package sticks to those. As a matter of fact, most good packages tend to peek into packages for other editors (VSCode, Atom, etc.) and see which scoping names they adopted, or consult online scoping references such as:

Compatibility with native color schemes shouldn't really be a concern in our case, because the AsciiDoc syntax is too different from the average programming languages, or even most markup syntaxes for that matter. IMO we should rather stick to good naming conventions that provide meaningful semantics, rather than stick to the rather poor official guidelines or comply with color schemes — which is why I chose to enforce the package color scheme in the first places.

If you look at the excellent Markdown package for ST (probably one of the most used and successful packages of ST) you'll see that it follows its own scoping conventions.

In our case, I think that the original markup.heading.level was intended to provide better semantics, because markup.heading should (at least in theory) be the entire scope of a section title, whereas in our case we're trying to scope just the = delimiter, which marks the heading level — in practice, though, in order to achieve Symbol indexing and other features (e.g. Goto Symbol) we're forced to stick with the few scopes that support such features, even if they are semantically at odds with the context being added — so much for the "semantic scoping guidelines" promoted by ST official documentation. If I remember correctly, these considerations where the reason why I hesitated so long in dropping the level segment.

One of the reasons why I stopped publishing packages on the official channel (although not the only one) was because I was tired of having to deal with their tweak requests and debate over the official guidelines and whether they make sense (i.e. beyond C++, Python, and the usual languages). From what I've seen, most good packages are deviating from such guidelines, because in order to achieve good editing functionality (e.g. via plugins and other custom scripts) you need to adopt scoping semantics with suit the need of the syntax at hand, rather than the vision of ST creators and maintainers.

Anyhow, as a general rule, I'd rather prefer that we discuss changes that revert the original design choices in the package, before submitting a PR containing such changes — usually that's how collaborative efforts work, through discussion and team planning.

You've been contributing many useful features to this package, hence I think that if we managed to discuss and plan the package design and its features (e.g. via Discussions) we'll be able to better coordinate. The task of reviving this very old (and, in many respects broken) package is quite a challenge, partly because we don't have many clues as to the original design goals, but mostly because the AsciiDoc syntax is really hard to tackle.

I'd really like the idea of having you aboard as a project collaborator, but this might entail coming up with collaborative strategies that please both — i.e. finding that agreement and set of compromises on how to go about changes that allows both of us freedom of action but, at the same time, prevents us from stopping on each other's work. From my experience with long-term collaborative projects, all it takes is discussing ideas and getting to know each other, so that everyone is on the same wavelength.

tajmone commented 6 months ago

@polyglot-jones, I realized that this PR was breaking how the color scheme treated block captions, e.g.

.some title
============================
example
============================

Since caption titles were being scoped as markup.heading.block, when you dropped the .level segment from headings scopes the color scheme was coloring also captions with blue background color (i.e. blue on blue), which rendered unreadable.

I've taken liberty to rewrite commits history in dev branch, and amended the commit of your PR so that the color scheme now excludes markup.heading.block from the rules targeting headings:

        // =====================================================================
        // SECTION TITLES
        // =====================================================================
        {
            "name": "Section Titles",
            "scope": "markup.heading -markup.heading.block",
            "background": "dodgerblue",
            "font_style": "bold",
        },

Again, this is to ensure that every commit that ends up in master branch will represent a valid state of the repository — keeping commits with incomplete states is not only inelegant but also risk, e.g. they might introduce broken states when cherry picking or rebasing.

So, bear in mind that I've force pushed on dev branch, and you'll need to rebase accordingly.