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

Docs for 3.0 (#148) #313

Merged
merged 36 commits into from Jan 17, 2018
Merged

Docs for 3.0 (#148) #313

merged 36 commits into from Jan 17, 2018

Conversation

denysdovhan
Copy link
Member

@denysdovhan denysdovhan commented Jan 15, 2018

We accomplished all the work related to code in #148. Now it's time to update docs.

@denysdovhan denysdovhan added the help-wanted We need help to resolve this issue or PR. label Jan 15, 2018
@denysdovhan denysdovhan added this to the v3.0 milestone Jan 15, 2018
@denysdovhan
Copy link
Member Author

@salmanulfarzy @maximbaz @nfischer any suggestions or just thoughts?

@denysdovhan denysdovhan mentioned this pull request Jan 15, 2018
25 tasks
@denysdovhan denysdovhan added the docs Issue or PR is related to documentation. label Jan 15, 2018
Copy link
Contributor

@maximbaz maximbaz left a comment

Choose a reason for hiding this comment

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

Already looking good, my questions/suggestions below.

* `?` — untracked changes;
* `+` — uncommitted changes in the index;
* `!` — unstaged changes;
* `✘` — deleted files;
* Prompt character turns red if the last command exits with non-zero code.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still true, keep it maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's now the second item of the list. Look up ☝️

Copy link
Member Author

Choose a reason for hiding this comment

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

@maximbaz please, click Resolve button to hide this thread.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have it...?

image

* Current Python pyenv (`🐍`).
* Current .NET SDK version, through dotnet-cli (`.NET`).
* Current Ember.js version, through ember-cli (`🐹`).
* Current Kubectl context (`☸️`).
* Package version, if there's is a package in current directory (`📦`).
Copy link
Contributor

Choose a reason for hiding this comment

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

"package" is too ambiguous, npm package?

Copy link
Member Author

Choose a reason for hiding this comment

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

This section is aimed to work not only with npm-packages.

docs/Options.md Outdated

### Time (`time`)

Disabled as default. Set `SPACESHIP_TIME_SHOW` to `true` in your `.zshrc`, if you need to show time stamps.
Copy link
Contributor

Choose a reason for hiding this comment

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

"by default" sounds a bit better imho, same for "exit code" section below

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

docs/Options.md Outdated

| Variable | Default | Meaning |
| :------- | :-----: | ------- |
| `SPACESHIP_USER_SHOW` | `true` | Show user section |
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you planning to add details that there are 4 allowed values for this variable and describe their meaning, and if so, where? Currently I see true, I would guess that the value could also be false, but I wouldn't know about the existence of always or needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. WIP.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm I didn't ask correctly, I was meaning to ask this: where are you planning to document all available options? Directly here, or somewhere in a different section of the documentation? A nested table of some kind?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think directly here. I'll figure out something.

docs/Options.md Outdated

### Battery (`battery`)

By default, Battery section is shown only if battery level is below `SPACESHIP_BATTERY_THRESHOLD` (default: 10%) or it's fully charged. It can be made always visible by setting `SPACESHIP_BATTERY_ALWAYS_SHOW=true`.
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no SPACESHIP_BATTERY_ALWAYS_SHOW anymore

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@salmanulfarzy
Copy link
Member

Presets wiki already show configurations, Where do you want example configuration ?

If you meant example for developing new sections, I think it would better fit in CONTRIBUTING.md. What do you think ?

@denysdovhan
Copy link
Member Author

@salmanulfarzy can you check API docs. Any suggestions?

Where do you want example configuration?

Either on API page or CONTRIBUTING.md. Where's the best place for it?

Copy link
Contributor

@maximbaz maximbaz left a comment

Choose a reason for hiding this comment

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

Looking good - maybe "Approve" resolves comments? 🙂

Copy link
Member

@salmanulfarzy salmanulfarzy left a comment

Choose a reason for hiding this comment

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

Changes in mercurial API and some suggestions

README.md Outdated
@@ -82,7 +79,7 @@ You can find more examples with different color schemes in [Screenshots](https:/

For correct work you will first need:

* A [`zsh`](http://www.zsh.org/) (v5.0.5 or recent) must be installed
* A [`zsh`](http://www.zsh.org/) (v5.0.6 or recent) must be installed
Copy link
Member

Choose a reason for hiding this comment

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

Even though 5.0.6 is known to work, I think it would be preferable to require latest version like 5.2.

  • Ubuntu 16.10 and above ships with 5.2
  • Ubuntu 16.04 LTS ships with 5.1.1
  • macOS 10.12 ships with 5.2
  • macOS 10.11 ships with 5.0.8

Copy link
Member Author

Choose a reason for hiding this comment

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

Why? If it works on lower versions, why wouldn't we keep them as required?

docs/API.md Outdated

## `spaceship::is_hg`

The same as [`spaceship::is_git`](#spaceshipisgit), but for functions. This utility returns zero exit code if a current working directory is a Git repository and non-zero if it's not.
Copy link
Member

Choose a reason for hiding this comment

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

  • but for mercurial repositories.
  • working directory is a Mercurial repository and non-zero

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

docs/API.md Outdated

### Arguments

1. `deprecated` _Required_ — the name of a deprecated variable. If this variable is set (contains any value), then `"%B$deprecated%b is deprecated.` will be printed.
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a note about %B...%b, So users would know what it is ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

docs/API.md Outdated

This command validates that given program is available for execution. It returns zero exit code if a `command` exists and non-zero if `command` doesn't.

You can use this utility to check if some program is installed and perform actions conditionally. For example, you can either return an error and exit or continue script's execution.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this instead of which? Does spaceship::exists foo succeed or fail if foo is a function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Check if a program exists from a Bash script
— here's quite comprehensive explanation why we should not rely on which.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is all true for bash, but not for zsh. In zsh:

% which which
which: shell built-in command

I see your implementation is built on top of command -v, which I believe will have the same basic behavior as which in zsh. I think the current implementation is fine, but it would be nice to document that spaceship::exists checks for PATH binaries, functions, and builtins.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@denysdovhan
Copy link
Member Author

Just finished with docs. I think it's ready to merge.

@salmanulfarzy @maximbaz and @nfischer, please, reread docs carefully to make sure we didn't miss anything important 🙏

After merging this PR, #148 is ready to release. 🎉 🎉 🎉

@denysdovhan denysdovhan changed the title WIP: Docs for 3.0 (#148) Docs for 3.0 (#148) Jan 17, 2018
Copy link
Contributor

@maximbaz maximbaz left a comment

Choose a reason for hiding this comment

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

Nice job, awesome to see 3.0 released 👏

docs/API.md Outdated

## `SPACESHIP_ROOT`

> **Attention!** Do not modify the value of this variable! Changing the value may cause the damage of Spaceship installation!
Copy link
Contributor

Choose a reason for hiding this comment

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

"damage to" may sound better

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

docs/Options.md Outdated

> Works only for npm at the moment. Please, help us improve this section!

Package version is shown when repository is a package (contains a `package.json` file). This is the version of the package you are working on, not the version of package manager itself.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since it will support not only npm packages, I'd spell it like this:

... (e.g. contains a `package.json file). Note: this is the version...

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

| `false` | Hidden | Hidden | Hidden |
| `always` | Shown | Shown | Shown |
| `true` | Shown | Hidden | Shown |
| `low` | Shown | Hidden | Hidden |
Copy link
Contributor

Choose a reason for hiding this comment

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

This is definitely optional, but I must say, I wish all of the tables were formatted like this one, it's so much nicer to look at properly formatted tables like this one comparing to the tables like the one above. Even thought they are both rendered identically, I'm usually reviewing a non-rendered version, just because it's easier to immediately leave comments.

Many editors have some kind of "table formatter" extensions to do this automatically.

Copy link
Member

Choose a reason for hiding this comment

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

I was just thinking about this and https://github.com/prettier/prettier does an awesome job formatting markdown documents. Have a look at salmanulfarzy@57fa72f and raw file here

Copy link
Contributor

@maximbaz maximbaz Jan 17, 2018

Choose a reason for hiding this comment

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

Thank you for this tip, definitely going to use it myself!

Just a note, prettier renders columns containing `` incorrectly (see for example Options.md:464), but I already reported this prettier/prettier#3753

UPDATE: also this: prettier/prettier#3754.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, but it's hard to maintain proper formatting for tables in long terms. Constant reformating to keep the same width for each row would kill git history related to these rows.

When I initially released Spaceship, I tried to maintain table formatting, but soon I realized that this doesn't play well for me.

Tools like Prettier would help much with maintaining formatting, but for now, it doesn't support shell scripts. I'd prefer to postpone adding Prettier right now.

* Install any powerline compatible font like [Fira Code](https://github.com/tonsky/FiraCode) or [others](https://github.com/powerline/fonts).
* Configure your terminal emulator to [use that font](https://powerline.readthedocs.io/en/master/troubleshooting/osx.html).

## What's the weird characters infront of sections?
Copy link
Contributor

Choose a reason for hiding this comment

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

"character in front of a section" sounds better

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

```
* Configure your terminal emulator to use UTF-8 as character encoding.

## Some of sections icons put on one another?
Copy link
Contributor

Choose a reason for hiding this comment

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

Some section icons overlap each other?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. Perks of being a non-native speaker 😁

* Check _Threat ambiguous-width characters as double-width_.
* Reload terminal's tab.

## Why my prompt doesn't work like on preview?
Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't my prompt look like the preview?

Copy link
Member

@salmanulfarzy salmanulfarzy left a comment

Choose a reason for hiding this comment

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

Great job @denysdovhan, Some minor suggestions.

docs/API.md Outdated

# Show foobar section only when there are foobar-specific files
# in current working direcotory
[[ -f foobar.conf || -n *.foo(#qN^/) || -n *.bar(#qN^/) ]] || return
Copy link
Member

Choose a reason for hiding this comment

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

Can we have a short note or link about glob qualifiers ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could make a spaceship API to perform -n *.<some extension>(#qN^/). This reduces lots of repetition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding API is an interesting idea, but let's postpone it. For now, I've added a link to docs about glob qualifiers.

* Symlink `spaceship.zsh` to somewhere in [`$fpath`](http://www.refining-linux.org/archives/46/ZSH-Gem-12-Autoloading-functions/) as `prompt_spaceship_setup`.
* Initialize prompt system and choose `spaceship`.

#### Example
Copy link
Member

Choose a reason for hiding this comment

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

Not mentioning prezto might confuse users about installation method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean adding a section on how to install it with prezto? But the theme is not even added yet in prezto-contrib, sorin-ionescu/prezto#1504

Copy link
Member

Choose a reason for hiding this comment

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

Most Zsh frameworks and Zsh plugin managers except prezto are specified in installation methods. spaceship wasn't compatible with prezto till 3.0, So users might assume the same.

It would be great if we at least specify how to install spaceship on prezto.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't use prezto. Can you add a section about installing for its users?

Copy link
Member

Choose a reason for hiding this comment

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

prezto follows vanilla Zsh prompt setup, Manual installation methods will get the job done. But we just haven't specified it anywhere.

# Summary

* [Home](README.md)
* [Options](./docs/Options.md)
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. ./SUMMARY.md is link to ./docs/README.md. GitBook uses SUMMARY.md as an entry point. Everything works well here.

Copy link
Member

Choose a reason for hiding this comment

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

Everything works well here.

Then great

docs/Options.md Outdated
| Variable | Default | Meaning |
| :------- | :-----: | ------- |
| `SPACESHIP_GIT_SHOW` | `true` | Show Git section |
| `SPACESHIP_GIT_PREFIX` | `on ` | Prefix before Git section |
Copy link
Contributor

@maximbaz maximbaz Jan 17, 2018

Choose a reason for hiding this comment

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

Markdown will strip the leading and trailing whitespaces in inline code blocks, so rendered documentation will be incorrect!

See this for context: #313 (comment) and prettier/prettier#3754

UPDATE: We can use   (U+00A0 NO-BREAK SPACE) to overcome this issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Using NO-BREAK SPACES don't help with GitBook. It still strips trailing space.

Copy link
Member Author

Choose a reason for hiding this comment

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

@maximbaz can you help with that?

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, but what's the plan then? Convert to no-break space and ignore that GitBook would still be incorrect?

Copy link
Member Author

Choose a reason for hiding this comment

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

@maximbaz let's fix this at least for GitHub. Please, keep spaces in code as is, replace only those in docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

all right, I'll do it in an hour, need to shortly leave now.

Copy link
Member Author

Choose a reason for hiding this comment

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

no problem 👌
waiting for you PR

Copy link
Member

@salmanulfarzy salmanulfarzy left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@denysdovhan
Copy link
Member Author

@salmanulfarzy @maximbaz please, review and make final approvement if everything is OK.

@maximbaz
Copy link
Contributor

See the comment about whitespaces being stripped, that's the only one I have

@denysdovhan
Copy link
Member Author

Merging this since changes were previously approved.

@denysdovhan denysdovhan merged commit 5dcd00e into 3.0 Jan 17, 2018
@denysdovhan denysdovhan deleted the 3.0-docs branch January 17, 2018 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Issue or PR is related to documentation. help-wanted We need help to resolve this issue or PR.
Development

Successfully merging this pull request may close these issues.

None yet

4 participants