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

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

Closed
wants to merge 3 commits into from

Conversation

polyglot-jones
Copy link
Collaborator

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.)

…out source code that has been re-implemented. (Left in a handful that haven't been re-implemented yet.)
@polyglot-jones
Copy link
Collaborator Author

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
Copy link
Owner

tajmone commented Jul 25, 2021

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).

@@ -7,20 +7,6 @@
# Syntax definition for Asciidoctor:
# -- https://asciidoctor.org/
# ------------------------------------------------------------------------------
# NOTE: This syntax was taken from @bsmith-n4, who slightly improved the
Copy link
Owner

Choose a reason for hiding this comment

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

If we remove this note we should then credit @bsmith-n4 in the main README. I'll create an Issue to remind me of this task, so that wen we merge all the changes into master the due credits are there also.

Copy link
Owner

Choose a reason for hiding this comment

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

Done, see Issue #25.

@tajmone tajmone mentioned this pull request Jul 25, 2021
@tajmone
Copy link
Owner

tajmone commented Jul 25, 2021

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
Copy link
Collaborator Author

@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
Copy link
Owner

tajmone commented Jul 28, 2021

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

Also commit 17eb3e0 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
Copy link
Owner

tajmone commented Jul 29, 2021

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.

@polyglot-jones polyglot-jones deleted the jones-01 branch July 29, 2021 02:27
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