Skip to content

Document contribution to the code along with coding standards #321

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

Merged
merged 38 commits into from
Dec 1, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
ecc1ca9
Fix Microsoft/vscode#37627 (#1368)
octref Nov 3, 2017
7c5778c
Version 0.7.0 of extension (#1381)
DonJayamanne Nov 9, 2017
9d1bf82
Update README.md
DonJayamanne Nov 9, 2017
ffba179
Update README.md
DonJayamanne Nov 9, 2017
905c713
sync fork with upstream
DonJayamanne Nov 10, 2017
acc2109
fix readme
DonJayamanne Nov 10, 2017
d470523
Merge branch 'master' of https://github.com/Microsoft/vscode-python
DonJayamanne Nov 16, 2017
d392e8b
merged upstream
DonJayamanne Nov 16, 2017
92f775f
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 20, 2017
32a6e53
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 21, 2017
4b30f2c
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 22, 2017
e396752
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 22, 2017
eff4792
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 28, 2017
4553c28
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 28, 2017
3c6520a
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 28, 2017
966e516
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 28, 2017
63d2d65
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 28, 2017
f6d469e
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 28, 2017
6ffff66
updated
DonJayamanne Nov 29, 2017
bbe87a5
updated with coding standards
DonJayamanne Nov 29, 2017
23fd9fe
fixed headings
DonJayamanne Nov 29, 2017
f6f066d
updated tasks.json
DonJayamanne Nov 29, 2017
1edddb9
udpated gulp
DonJayamanne Nov 29, 2017
28c3028
always handle errors
DonJayamanne Nov 29, 2017
b5abe4a
oops
DonJayamanne Nov 29, 2017
9f83347
use node 8.9.1 and npm 5.5.1
DonJayamanne Nov 29, 2017
7b5c8a4
use any for options
DonJayamanne Nov 29, 2017
a382f88
fix pre launch task name
DonJayamanne Nov 29, 2017
029e055
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 30, 2017
e8c71c0
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 30, 2017
51cf9d2
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 1, 2017
60382a5
merged master
DonJayamanne Dec 1, 2017
4d46790
updated
DonJayamanne Dec 1, 2017
2158029
remove copyright check
DonJayamanne Dec 1, 2017
7aadc43
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 1, 2017
e33ef0b
merged
DonJayamanne Dec 1, 2017
1233177
updated gulp
DonJayamanne Dec 1, 2017
bfdcb01
fixed code review comments
DonJayamanne Dec 1, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ addons:
- g++-4.9-multilib
- libgtk2.0-0
- libx11-dev
- libxkbfile-dev
- libxkbfile-dev
- libsecret-1-dev
- python-dev
matrix:
Expand Down Expand Up @@ -41,8 +41,9 @@ before_install: |
git submodule update --init --recursive
git clone https://github.com/creationix/nvm.git ./.nvm
source ./.nvm/nvm.sh
nvm install 7.2.1
nvm use 7.2.1
nvm install 8.9.1
nvm use 8.9.1
npm i -g [email protected]
npm config set python `which python`
if [ "$TRAVIS_OS_NAME" == "osx" ]; then
pyenv install $PYTHON
Expand All @@ -53,6 +54,6 @@ install:
- pip install --upgrade -r requirements.txt
- npm install
- npm run vscode:prepublish

script:
- npm test --silent
9 changes: 5 additions & 4 deletions .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
"outFiles": [
"${workspaceFolder}/out/**/*.js"
],
"preLaunchTask": "compile"
"preLaunchTask": "Compile"
},
{
"name": "Launch Extension as debugServer", // https://code.visualstudio.com/docs/extensions/example-debuggers
Expand All @@ -30,7 +30,8 @@
"outFiles": [
"${workspaceFolder}/out/client/**/*.js"
],
"cwd": "${workspaceFolder}"
"cwd": "${workspaceFolder}",
"preLaunchTask": "Compile"
},
{
"name": "Launch Tests",
Expand All @@ -47,7 +48,7 @@
"outFiles": [
"${workspaceFolder}/out/**/*.js"
],
"preLaunchTask": "compile"
"preLaunchTask": "Compile"
},
{
"name": "Launch Multiroot Tests",
Expand All @@ -64,7 +65,7 @@
"outFiles": [
"${workspaceFolder}/out/**/*.js"
],
"preLaunchTask": "compile"
"preLaunchTask": "Compile"
}
],
"compounds": [
Expand Down
4 changes: 2 additions & 2 deletions .vscode/tasks.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"script": "compile",
"isBackground": true,
"problemMatcher": [
"$tsc",
"$tsc-watch",
{
"base": "$tslint5",
"fileLocation": "relative"
Expand All @@ -36,7 +36,7 @@
"panel": "shared"
},
"problemMatcher": [
"$tsc",
"$tsc-watch",
{
"base": "$tslint5",
"fileLocation": "relative"
Expand Down
46 changes: 46 additions & 0 deletions CODING_STANDARDS.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
## Coding guidelines for TypeScript
Copy link
Member

Choose a reason for hiding this comment

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

Are these taken from somewhere? I.e. is there a link to provide that mirrors these so that we don't have to maintain our own style guide? Or maybe one to fall back on when this style guide doesn't answer a question?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

@brettcannon updated, let me know if the updated version is good.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, if the current style is divergent from the TS style guide then it LGTM.

* The following standards are inspired from [Coding guidelines for TypeScript](https://github.com/Microsoft/TypeScript/wiki/Coding-guidelines).

### Names
* Use PascalCase for type names.
* Use "I" as a prefix for interface names only when an interface is implemented by a class.
* Use PascalCase for enum values.
* Use camelCase for function names.
* Use camelCase for property names and local variables.
* Do not use "_" as a prefix for private properties (unless used as backing properties).
* Use whole words in names when possible.

### Types

* Do not export types/functions unless you need to share it across multiple components.
* Do not introduce new types/values to the global namespace.
* Shared types should be defined in 'types.ts'.
Within a file, type definitions should come first.

### null and undefined

Use undefined. Do not use null.

### Comments

Use JSDoc style comments for functions, interfaces, enums, and classes.

### Strings

Use single quotes for strings.

### Style

* Use arrow functions over anonymous function expressions.
* Always surround loop and conditional bodies with curly braces. Statements on the same line are allowed to omit braces.
* Open curly braces always go on the same line as whatever necessitates them.
* Parenthesized constructs should have no surrounding whitespace.
* A single space follows commas, colons, and semicolons in those constructs. For example:
* `for (var i = 0, n = str.length; i < 10; i++) { }`
* `if (x < 10) { }`
* `function f(x: number, y: string): void { }`

* `else` goes on a the same line from the closing curly brace.
* Use 4 spaces per indentation.


49 changes: 34 additions & 15 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -1,45 +1,64 @@
## Contribution
* Please feel free to fork and submit pull requests
* Feature requests can be added [here](https://github.com/DonJayamanne/pythonVSCode/issues/183)

## Prerequisites
1. Node.js
### Prerequisites

1. Node.js (>= 8.9.1, < 9.0.0)
2. Python 2.7 or later (required only for testing the extension and running unit tests)
3. Windows, OS X or Linux
4. Visual Studio Code
5. Following VS Code extensions:
* [TSLint](https://marketplace.visualstudio.com/items?itemName=eg2.tslint)
* [EditorConfig for VS Code](https://marketplace.visualstudio.com/items?itemName=EditorConfig.EditorConfig)

### Setup

## Setup
```
git clone https://github.com/DonJayamanne/pythonVSCode
cd pythonVSCode
git clone https://github.com/microsoft/vscode-python
cd vscode-python
npm install
```
## Development workflow

### Incremental Build
Run the build Task from the [Command Palette](https://code.visualstudio.com/docs/editor/tasks) (short cut CTRL+SHIFT+B or ⇧⌘B)

Run the `Compile` and `Hygiene` build Tasks from the [Command Palette](https://code.visualstudio.com/docs/editor/tasks) (short cut `CTRL+SHIFT+B` or `⇧⌘B`)

### Errors and Warnings
TypeScript errors and warnings will be displayed in VS Code in the Problems Panel (CTRL+SHIFT+M or ⇧⌘M)

TypeScript errors and warnings will be displayed in VS Code in the following areas:
* Problems Panel (`CTRL+SHIFT+M` or `⇧⌘M`)
* Terminal running the `Compile` task
* Terminal running the `Hygiene` task

### Validate your changes

To test the changes you launch a development version of VS Code on the workspace vscode, which you are currently editing.
Use the "Launch Extension" launch option.
Use the `Launch Extension` launch option.

### Unit Tests
Run the Unit Tests via the "Launch Test" launch option.
Currently unit tests only run on [Travis](https://travis-ci.org/DonJayamanne/pythonVSCode)

Run the Unit Tests via the `Launch Test` and `Launch Multiroot Tests` launch option.
Currently unit tests only run on [Travis](https://travis-ci.org/Microsoft/vscode-python)

_Requirements_
1. Ensure you have disabled breaking into 'Uncaught Exceptions' when running the Unit Tests
2. For the linters and formatters tests to pass successfully, you will need to have those corresponding Python libraries installed locally

## Debugging the extension
### Standard Debugging

Clone the repo into any directory and start debugging.
From there use the "Launch Extension" launch option.
From there use the `Launch Extension` launch option.

### Debugging the Python Extension Debugger

The easiest way to debug the Python Debugger (in our opinion) is to clone this git repo directory into [your](https://code.visualstudio.com/docs/extensions/install-extension#_your-extensions-folder) extensions directory.
From there use the ```Launch Extension as debugserver``` launch option.
From there use the ```Extension + Debugger``` launch option.

### Coding Standards

Information on our coding standards can be found [here](https://github.com/Microsoft/vscode-python/blob/master/CODING_STANDARDS.md).
We have a pre-commit hook to ensure the code committed will adhere to the above coding standards.


## Development process

Expand Down
89 changes: 69 additions & 20 deletions gulpfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const colors = require('colors/safe');
* named according to the checks performed on them. Each subset contains
* the following one, as described in mathematical notation:
*
* all ⊃ eol ⊇ indentation ⊃ copyright ⊃ typescript
* all ⊃ eol ⊇ indentation ⊃ typescript
*/

const all = [
Expand Down Expand Up @@ -115,12 +115,12 @@ const hygiene = (some, options) => {
.toString('utf8')
.split(/\r\n|\r|\n/)
.forEach((line, i) => {
if (/^\s*$/.test(line)) {
if (/^\s*$/.test(line) || /^\S+.*$/.test(line)) {
// Empty or whitespace lines are OK.
} else if (/^(\s\s\s\s)+.*/.test(line)) {
// Good indent.
} else if (/^[\t]+.*/.test(line)) {
console.error(file.relative + '(' + (i + 1) + ',1): Bad whitespace indentation');
console.error(file.relative + '(' + (i + 1) + ',1): Bad whitespace indentation (use 4 spaces instead of tabs or other)');
errorCount++;
}
});
Expand All @@ -137,7 +137,7 @@ const hygiene = (some, options) => {
tsfmt: true
}).then(result => {
if (result.error) {
console.error(result.message);
console.error(result.message.trim());
errorCount++;
}
cb(null, file);
Expand All @@ -147,14 +147,14 @@ const hygiene = (some, options) => {
});
});

const program = require('tslint').Linter.createProgram("./tsconfig.json");
const linter = new tslint.Linter(options, program);
const tsl = es.through(function (file) {
const configuration = tslint.Configuration.findConfiguration(null, '.');
const options = {
formatter: 'json'
};
const contents = file.contents.toString('utf8');
const program = require('tslint').Linter.createProgram("./tsconfig.json");
const linter = new tslint.Linter(options, program);
linter.lint(file.relative, contents, configuration.results);
const result = linter.getResult();
if (result.failureCount > 0 || result.errorCount > 0) {
Expand Down Expand Up @@ -206,22 +206,16 @@ const hygiene = (some, options) => {
.pipe(filter(f => !f.stat.isDirectory()))
.pipe(filter(eolFilter))
.pipe(options.skipEOL ? es.through() : eol)
.pipe(filter(indentationFilter));

if (!options.skipIndentationCheck) {
result = result
.pipe(indentation);
}
.pipe(filter(indentationFilter))
.pipe(indentation);

// Type script checks.
let typescript = result
.pipe(filter(tslintFilter));
.pipe(filter(tslintFilter))
.pipe(formatting);

if (!options.skipFormatCheck) {
typescript = typescript
.pipe(formatting);
}
typescript = typescript.pipe(tsl)
typescript = typescript
.pipe(tsl)
.pipe(tscFilesTracker)
.pipe(tsc());

Expand All @@ -244,16 +238,32 @@ gulp.task('hygiene-staged', () => run({ mode: 'changes' }));
gulp.task('hygiene-watch', ['hygiene-staged', 'hygiene-watch-runner']);

gulp.task('hygiene-watch-runner', function () {
/**
* @type {Deferred}
*/
let runPromise;

return watch(all, { events: ['add', 'change'] }, function (event) {
// Damn bounce does not work, do our own checks.
const start = new Date();
if (runPromise && !runPromise.completed) {
console.log(`[${start.toLocaleTimeString()}] Already running`);
return;
}
console.log(`[${start.toLocaleTimeString()}] Starting '${colors.cyan('hygiene-watch-runner')}'...`);

runPromise = new Deferred();
// Skip indentation and formatting checks to speed up linting.
return run({ mode: 'watch', skipFormatCheck: true, skipIndentationCheck: true })
run({ mode: 'watch', skipFormatCheck: true, skipIndentationCheck: true })
.then(() => {
const end = new Date();
const time = (end.getTime() - start.getTime()) / 1000;
console.log(`[${end.toLocaleTimeString()}] Finished '${colors.cyan('hygiene-watch-runner')}' after ${time} seconds`);
});
runPromise.resolve();
})
.catch(runPromise.reject.bind);

return runPromise.promise;
});
});

Expand Down Expand Up @@ -402,7 +412,46 @@ function getModifiedFiles() {
});
});
}

// this allows us to run hygiene as a git pre-commit hook.
if (require.main === module) {
run({ exitOnError: true, mode: 'staged' });
}

class Deferred {
constructor(scope) {
this.scope = scope;
this._resolved = false;
this._rejected = false;

this._promise = new Promise((resolve, reject) => {
this._resolve = resolve;
this._reject = reject;
});
}
resolve(value) {
this._resolve.apply(this.scope ? this.scope : this, arguments);
this._resolved = true;
}
/**
* Rejects the promise
* @param {any} reason
* @memberof Deferred
*/
reject(reason) {
this._reject.apply(this.scope ? this.scope : this, arguments);
this._rejected = true;
}
get promise() {
return this._promise;
}
get resolved() {
return this._resolved === true;
}
get rejected() {
return this._rejected === true;
}
get completed() {
return this._rejected || this._resolved;
}
}