search

tajmone / ST4-Asciidoctor

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

Syntax Highlighting work. First of many (atomic) commits into dev instead of master. #20

Closed polyglot-jones closed 3 years ago

polyglot-jones commented 3 years ago

Comments only. Fixed a couple of typos. Deleted all of the commented-out source code that has been re-implemented. (Left in a handful that haven't been re-implemented yet.)

polyglot-jones commented 3 years ago

Weird. It says...

The following files didn't pass the validation test:
Syntaxes/Asciidoctor.sublime-syntax
Run ECLint locally for detailed information about the problems.

But when I run ECLint locally, all is fine. Only if I force an error in Asciidoctor.sublime-syntax (e.g. making an indent 3 spaces instead of 2) can I get ECLint to complain.

tajmone commented 3 years ago

But when I run ECLint locally, all is fine. Only if I force an error in Asciidoctor.sublime-syntax (e.g. making an indent 3 spaces instead of 2) can I get ECLint to complain.

EClint is a very buggy tool, and the project has officially been dead since years now. So its behavior might differ from one OS to another, and it tends to clog-up under Windows. Unfortunately, I haven't been able to find a better alternative yet; I did spot a promising tool, but I've been testing it in production and found many Windows specific bugs, which I reported, so I'm waiting that all bugs are fixed before switching to that.

But the ST EditorConfig package should abide to the settings anyway, so I don't understand why the code style violations are there in the first place — GitHub diff previews are highlighting some of these, since GH supports EditorConfig settings (although differently from EClint).

tajmone commented 3 years ago

I originally added all those huge (and ugly) comment block in an effort to simplify comparing the fixes found on package forks into the original, not just in terms of different RegExs or approaches, but most of all because I noticed that the forks were using alternative scope names.

I've now lost track of which of those comments are still needed in the source, so if you say they're sage to go since they are integrated I'm fine with it (can always look them upstream up if I have doubts, and they're probably outdated too).

If you can't manage to fix the EClint warnings via the help of the EditorConfig package, I'll just merge the PR and try to fix the error locally. The important thing is that all Travis builds pass at least on master.

polyglot-jones commented 3 years ago

@tajmone

If you can't manage to fix the EClint warnings via the help of the EditorConfig package, I'll just merge the PR and try to fix the error locally.

Nope. Didn't help. So, you'll have to do that.

tajmone commented 3 years ago

As in PR #22, Isn't commit 043535e01460 (GIT Ignore *.chback files.) the same one that was just merged in a previous PR (#21)?

Also commit 17eb3e0fb135 is present in PR #21 (see review note above), and should go.

These commits are redundant and should be removed from this branch to keep history linear.

tajmone commented 3 years ago

This PR could have been fixed without closing it.

I'm not sure where these spurious commits are coming from, but I'm guessing that your fork of the repository has also additional remotes pointing to other forks, which is also what I do to integrate changes from third party forks.

In that case, in my experience, the easiest way to integrate changes from one remote to another, without disrupting history linearity, is to create a new branch form the dev branch of the PR target repo (this one, in our case) and then simply use git checkout to bring into the working stage the individual files from the other remote (i.e. after having created a local branch from it). This way it's easier to just focus on the specific files to include in the PR and, unlike cherry picking commits, you still have time to edit those file before committing them for the PR.

For the same reasons, I believe that each PR should focus on single feature, otherwise its hard to reconciliate the different changes from different remotes.

At least, this has been my personal experience when working on my own fork of a repository which also has other third party forks which I'm interested in following and picking changes from. I also believe that this will eventually turn out to be a good strategy if we ever manage to coordinate efforts to merge different forks of this package into a unified repository that will replace all forks — having a linear history is important, especially when trying to track changes history via feature like blame, where clean-cut commits per-feature make it easier to track what was done when, where and by who.