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

Cleanup markdown sample code #408

Merged
merged 4 commits into from
Feb 15, 2018
Merged

Cleanup markdown sample code #408

merged 4 commits into from
Feb 15, 2018

Conversation

christopherjbaker
Copy link
Contributor

This pull request was created with Landscaper.
The following code mods were used to create this PR:

  1. https://gist.github.com/christopherjbaker/49b7e985b37032902d68a17dd2daa070

Please review this PR carefully as Landscaper does not guarantee any code mod's quality.

can-connect.md Outdated
var dataUrl = require("can-connect/data/url/");
var constructor = require("can-connect/constructor/");
var todosConnection = connect([dataUrl, constructor],{
```javascript
Copy link
Member

Choose a reason for hiding this comment

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

Why did this change from just js?

Copy link
Contributor Author

@christopherjbaker christopherjbaker Feb 13, 2018

Choose a reason for hiding this comment

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

consistency. no particular reason. there were lots of js and lots of javascript, so I just picked one.

Copy link
Member

Choose a reason for hiding this comment

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

This is fine, but note that even some of our own tools don’t support javascript.

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 don't really mind either way. They use js now.

can-connect.md Outdated
require("can-connect/constructor/constructor"),
require("can-connect/data/url/url")
const todoConnection = connect([
require("can-connect/constructor/constructor"),
Copy link
Member

Choose a reason for hiding this comment

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

I think these should be tabs and not spaces to be consistent with what’s in our style guide.

Copy link
Contributor Author

@christopherjbaker christopherjbaker Feb 13, 2018

Choose a reason for hiding this comment

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

While I would normally agree, I thought I had read somewhere that you shouldn't use tabs in markdown code. Am i mistaken?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to make this change, we need to update all of the .editorconfig files to make sure spaces are used in .md files. But it is still going to be impossible to use the correct spacing in inline docs inside of .js files. For that reason alone, I would say we should not make this change. I haven't seen any problems with using tabs in markdown. Can you provide a link?

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 don't have a link, and searching did not turn one up. I just recall it being mentioned last time the tabs-vs-spaces argument came up. It makes to difference to me though. They use tabs now.

@christopherjbaker christopherjbaker changed the base branch from md-code-fixes to master February 14, 2018 01:10
@christopherjbaker christopherjbaker changed the title Use const/let in sample code Cleanup markdown sample code Feb 14, 2018
@christopherjbaker christopherjbaker merged commit 9289fe9 into master Feb 15, 2018
@christopherjbaker christopherjbaker deleted the landscaper/3846-es6 branch February 15, 2018 00:55
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.

3 participants