Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

[docs] adds optional capability to provide code examples in rules' metadata #3602

Merged
merged 12 commits into from
Apr 19, 2018

Conversation

aervin
Copy link
Contributor

@aervin aervin commented Dec 21, 2017

PR checklist

  • Addresses an existing issue: #3601
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

Overview of change:

Adds optional capability to provide code examples (passing and failing) for a given rule.

@aervin aervin changed the title codeExample support init implementation [docs] adds optional capability to provide code examples in rules' metadata Dec 21, 2017
@@ -5,6 +5,15 @@
{% if page.descriptionDetails %}
{{page.descriptionDetails | markdownify}}
{% endif %}

{% if page.codeExamples %}
<h3>Code examples: </h3>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Code examples (default config)"

@@ -72,6 +73,17 @@ export class Rule extends Lint.Rules.AbstractRule {
type: "functionality",
typescriptOnly: false,
hasFix: true,
codeExamples: {
pass: dedent`
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this also needs to support multiple examples for different configurations / options.

codeExamples: {
  default: {
    pass: "...",
    fail: "...",
  },
  "ignore-same-line": { ... }, 
} 

Or maybe just an array with an optional description per object.

Copy link
Contributor Author

@aervin aervin Dec 26, 2017

Choose a reason for hiding this comment

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

From my limited experience with the doc portion of the codebase, I think an array would be simplest. So

codeExamples?: {
  config: string; /* Or... ? */
  pass: string;
  fail: string;
  description?: string; /* Optional for cases like example below */
}[]

I think an explicit config example would be a good idea from the doc user's perspective, but will probably cause some metadata bloat in some rules.

{
  "rules": { "no-console": true }
}

[edit] forgot to include description

@aervin
Copy link
Contributor Author

aervin commented Jan 14, 2018

@ajafff I think this one is ready for further consideration when you find time.

@Shinigami92
Copy link
Contributor

Shinigami92 commented Jan 14, 2018

How can I help and add examples to all rules? Should I wait on the merged request and then open a fork and make a branch, or should I make a fork of this current branch in the fork of @aervin?
And how am I able to show up the docs pages local in my browser? It's something with ruby I think?

@aervin
Copy link
Contributor Author

aervin commented Jan 14, 2018

@Shinigami92 I'd hold off on adding examples to old rules as these updates are still subject to change. Eventually, I think it'd be great to update the old rules with code examples like you suggest.

You will need Ruby to build the docs locally. With bundle installed, I'm able to cd into the docs directory and run bundle exec jekyll serve to view local updates. It took me some time to figure out how the source is compiled into Jekyll data. Whenever I make changes to TSLint source files, I run

yarn compile && yarn docs && bundle exec jekyll serve

to ensure that changes in the source metadata are reflected in the Jekyll data.

@Shinigami92
Copy link
Contributor

I will try this when I'm back home. Thx

@Shinigami92
Copy link
Contributor

Shinigami92 commented Jan 14, 2018

Here is an installation and how to run the docs in Windows (10):

git clone https://github.com/<username>/tslint.git
cd .\tslint\

npm install -g yarn
npm install
yarn compile
yarn docs

# From here -> Windows 10 Ubuntu Terminal
cd /mnt/<win-drive-letter>/<path-to-project>/tslint/docs
sudo apt-add-repository ppa:brightbox/ruby-ng
sudo apt-get update
sudo apt-get install zlib1g-dev ruby2.3 ruby2.3-dev build-essential
# zlib1g-dev is required for gem nokogiri-1.8.1
sudo gem update
sudo gem install jekyll bundler
bundle install
bundle exec jekyll serve

maybe this can be placed in a file in the project to help other devs ^^

helpfull link: https://jekyllrb.com/docs/windows/

@Shinigami92
Copy link
Contributor

I have started to create some examples.
At the end, please look over my english sentenses, because I'm not a native english speaker ^^ (german)

You can track my changes here:
aervin/tslint@docs/code-examples...Shinigami92:docs/code-examples

@Shinigami92
Copy link
Contributor

Shinigami92 commented Jan 15, 2018

@aervin You should respect the code-style of the file:
master...Shinigami92:docs/code-examples
scripts/buildDocs.ts
maybe revert some of the changes in this file

But I don't know why ci/circleci: testNext failes
it has nothing to do with the branch?!

@palantirtech, @ajafff or someone else can check the branch now and make an approved review so then others can add examples to other rules :)

@aervin
Copy link
Contributor Author

aervin commented Jan 15, 2018

@Shinigami92 Can you please clarify, I'm not sure what you mean by "You should respect the code-style of the file"? And the link doesn't take me to a file or anything.

@Shinigami92
Copy link
Contributor

@aervin fixed the link: I mean the file scripts/buildDocs.ts

@aervin
Copy link
Contributor Author

aervin commented Jan 15, 2018

@Shinigami92 I did not make those changes directly. I'm thinking it's from compiling/rebuilding the docs (there's probably a --fix in the process somewhere). Those examples in the photo all violate the project's TSLint config, and all those violations have fixers.

@Shinigami92
Copy link
Contributor

@aervin: Dont forgett to accept this PR: aervin#2
It's only from my fork to your fork ^^

@aervin
Copy link
Contributor Author

aervin commented Jan 15, 2018

@Shinigami92 Once we get confirmation on these doc updates, I'll make that a top priority.

@aervin aervin mentioned this pull request Mar 20, 2018
@aervin
Copy link
Contributor Author

aervin commented Apr 8, 2018

@suchanlee or @johnwiseheart can we squeeze this one in? I'm happy to make changes.

@suchanlee
Copy link
Contributor

@aervin thanks for your patience. I'll try to review this as soon as I can.

background: transparent;
}
}
& .code-example.pass {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the code style in this file suggests that & . should be &. and that there should be a new line before each of the blocks. same for &: (applies to all code changes in this file)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand about the new lines, but there's a difference between & .code-example and &.code-example, isn't there? Still, I think the &s are unnecessary here (the way they're being used). I'll just remove them entirely.

}
}
& .code-example.pass {
background-color: #f1fff1 !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

why is !important needed here and below?
if they're required, please leave a comment on why they're required. If not, remove.
Also, it would be awesome to have the color as a comment (e.g. light red or light green)

@@ -148,7 +148,7 @@ function buildSingleModuleDocumentation(documentation: IDocumentation, modulePat
// tslint:disable-next-line:no-var-requires
const module = require(modulePath);
const DocumentedItem = module[documentation.exportName] as Documented;
if (DocumentedItem != null && DocumentedItem.metadata != null) {
if (DocumentedItem !== null && DocumentedItem.metadata !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i'd prefer that it stay as != instead of !==. != null checks for both undefined and null whereas with !== we specify for null, which may not be the desired behavior. But happy to revisit if there's a good reason to change to !==!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this was changed as a result of the doc-building process (a pass with --fix maybe?). Anyway, I would never make these sorts of changes deliberately (in a PR).

{{ codeExample.pass | markdownify }}
</div>
{% if codeExample.fail %}
<h5 style="color: #d14;">Fails</h5>
Copy link
Contributor

Choose a reason for hiding this comment

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

id prefer all styles in css and outside of markup

<h4>{{ codeExample.description }}</h4>
{{ codeExample.config | markdownify }}
<h5>Passes</h5>
<div class="code-example pass">
Copy link
Contributor

Choose a reason for hiding this comment

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

one class naming rule I like is for "decorating" class names, in this case pass and below fail, is to preface them with - (e.g. -pass, -fail) so that css rules for pass or fail wouldn't apply. By prefacing decorator class names with dashes, you know that it wouldn't be affected by other styles since all other styles for prefaced class names would be scoped to a particular class name.

@suchanlee
Copy link
Contributor

suchanlee commented Apr 9, 2018

is the above screenshot up to date? in particular, is all the text still green? (doesn't seem to be that way in the code)

update: looks like all the headers are green for tslint docs. But it looks funky with the green header for fail hmm..

@aervin
Copy link
Contributor Author

aervin commented Apr 9, 2018

That picture above is out of date @suchanlee. The latest push looks something like this:
tslint-code-example

@@ -72,6 +72,58 @@ export class Rule extends Lint.Rules.AbstractRule {
type: "functionality",
typescriptOnly: false,
hasFix: true,
codeExamples: [
Copy link
Contributor

Choose a reason for hiding this comment

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

These code examples are going to end up taking a lot of space in the source files (good!). What are your thoughts on starting a precedent of putting them in a ruleName.examples.ts file that just exports an object with examples?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JoshuaKGoldberg I had a similar concern myself. I think the separate file approach makes sense 👍 and will update the curly rule example.

@@ -0,0 +1,238 @@
GEM
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be checked in to the repo? I'm not familiar with gem/gemfile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@suchanlee It looks like the bundler docs suggest checking in the lock file. But full disclosure, I'm no Ruby expert.

Copy link
Contributor

@suchanlee suchanlee left a comment

Choose a reason for hiding this comment

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

Thanks @aervin! Super stoked to get this in.

@suchanlee suchanlee merged commit 5c86dd4 into palantir:master Apr 19, 2018
@aervin
Copy link
Contributor Author

aervin commented Apr 20, 2018

Thanks for your help @suchanlee 👍

@suchanlee
Copy link
Contributor

Np!

@aervin aervin mentioned this pull request Apr 23, 2018
4 tasks
@aervin
Copy link
Contributor Author

aervin commented May 3, 2018

@Shinigami92 You did a lot of great work updating the docs on your fork--any interest in opening a PR with those changes?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants