Skip to content

Miscellaneous fixes, part 5 #18

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

savetheclocktower
Copy link
Collaborator

This started out as a focused PR and then gained scope when I decided to do one final “pass” through the docs in preparation for a release. It's time to get these docs out of beta!

Here's what's in this PR, and I apologize in advance for how schizophrenic it is:

  1. Updated build instructions — not comprehensive, but better than they were before. Removed some inaccurate guidance and made things feel less dated (hopefully).
  2. Updated “creating a modern Tree-sitter grammar” docs. The original flight-manual–style docs I wrote for the whole system are more than a year old at this point, and some things needed revisiting.
  3. There was a distracting mix of straight and curly apostrophes (' vs ’), so I standardized all the apostrophes (outside of code blocks) to be curly.
  4. A few screenshots were distractingly old or showed Atom branding instead of Pulsar. And the screenshots that explain how to do performance profiling were incredibly outdated; profiling now has a completely new UI in the Chromium dev tools. I replaced them with new screenshots.
  5. As always, I end up making some phrasing changes whenever I revisit an old file.

Push back on anything that feels wrong.

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Luckily the majority of this PR was easy to review due to the small yet consistent changes.

For situations where that's not the case I did my best to fully read through and ensure the information matched what was accurate and our tone.

I've added some comments in areas where I feel we may be missing that, and would love your thoughts.
Nothing here is really big enough to be a blocker, so we can likely move on without if we have to


## Set up dependencies

You should install a compatible version of Node. Read the `.nvmrc` file to learn which version of Node is best to use. If you don’t have one yet, you’ll probably find it useful to install a tool like <span class="platform-linux platform-mac"><a href="https://github.com/nvm-sh/nvm">NVM</a> or <a href="https://asdf-vm.com/">asdf</a></span><span class="platform-win"><a href="https://github.com/coreybutler/nvm-windows">NVM for Windows</a> or <a href="https://github.com/Schniz/fnm">fnm</a></span> to manage multiple versions of Node.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
You should install a compatible version of Node. Read the `.nvmrc` file to learn which version of Node is best to use. If you don’t have one yet, you’ll probably find it useful to install a tool like <span class="platform-linux platform-mac"><a href="https://github.com/nvm-sh/nvm">NVM</a> or <a href="https://asdf-vm.com/">asdf</a></span><span class="platform-win"><a href="https://github.com/coreybutler/nvm-windows">NVM for Windows</a> or <a href="https://github.com/Schniz/fnm">fnm</a></span> to manage multiple versions of Node.
You should install a compatible version of Node. Read the `.nvmrc` file to learn which version of Node is best to use. If you don’t have one yet, you’ll probably find it useful to install a tool like <span class="platform-linux platform-mac"><a href="https://github.com/nvm-sh/nvm">NVM</a> or <a href="https://asdf-vm.com/">asdf</a></span><span class="platform-win"><a href="https://github.com/coreybutler/nvm-windows">NVM for Windows (Although if you need to build native packages, you may be better off manually installing the version of Node you need)</a> or <a href="https://github.com/Schniz/fnm">fnm</a></span> to manage multiple versions of Node.

Just wanting to add some kind of warning here, since for most node managers I've had trouble getting node-gyp to respect what they set as the installed version. I've never looked too much into it, so there may be a solution we could document, but for the time being this may be smart to just give a small warning.

@@ -12,3 +12,5 @@ Any misspelled words will be highlighted (by default with a dashed red line bene
Spell-checking is implemented in the {spell-check} package. You can visit this package’s settings page to customize the list of grammars for which spell check is enabled, add exclusions and known words — or to disable the package entirely.

The default grammars to spell check are `text.plain`, `source.gfm`, `text.git-commit`, `source.asciidoc`, `source.rst`, and `text.restructuredtext` — but you can add any other grammars for other file types you wish to check.

You can also add arbitrary scopes within documents that should be spell-checked. For instance: you can add `source comment` as a scope if all comments within all sorts of source files should be spell-checked.
Copy link
Member

Choose a reason for hiding this comment

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

Glad to see you've added my favourite spell-check addition here, good to have it documented!

@@ -54,7 +58,7 @@ Suppose you want to customize the color of a variable in Python. You can use the

If we hard-coded a color value here, we’d probably want to change the value if we switched syntax themes in the future. Instead, we can use one of the variables that syntax themes must define; that way the rule will work no matter which syntax theme is active.

Here’s [the full list of syntax variables](https://github.com/pulsar-edit/pulsar/blob/master/static/variables/syntax-variables.less) you can rely on.
Here’s [the full list of syntax variables](https://github.com/pulsar-edit/pulsar/blob/master/static/variables/syntax-variables.less) you can rely on existing in most syntax themes.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Here’s [the full list of syntax variables](https://github.com/pulsar-edit/pulsar/blob/master/static/variables/syntax-variables.less) you can rely on existing in most syntax themes.
Here’s [the full list of syntax variables](https://github.com/pulsar-edit/pulsar/blob/master/static/variables/syntax-variables.less) you can rely on being present in most syntax themes.

May just be me, but I had to re-read this sentence a second time to get what you mean. It may just be the break when looking at in MD, but this feels a bit more clear

@@ -306,7 +333,8 @@ Some tests take a single argument…
(#is? test.lastOfType))
```

Consult the [`ScopeResolver` API documentation](TODO) for a full list.
<!-- TODO: Replace with actual API documentation for `ScopeResolver` once we figure out how to express it. -->
Consult the [`ScopeResolver` API documentation](https://github.com/pulsar-edit/pulsar/blob/df0050f7b2f47448061895d14269ff97c4bae29f/src/scope-resolver.js#L608-L632) for a full list.
Copy link
Member

Choose a reason for hiding this comment

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

If these links are intended to be always up to date, instead of a commit sha, should we instead use HEAD? Using caps "HEAD" will ensure it always points to the most recent, and always points to whatever the default branch is. If we move the file the link will break, but this does mean it'll be constantly up to date till then.

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