Skip to content

Improve contributing guide and workflow #114

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 5 commits into from
Jul 6, 2020
Merged

Conversation

MaximeKjaer
Copy link
Contributor

This PR:

  • Fixes the contributing guide on the topic of the build artifact (see Commit auto-generated tmLanguage.json #23),
  • Moves all build commands to be NPM / Yarn scripts defined in package.json,
  • Removes an unused devDependency

This should simplify the workflow little, as there is a Yarn task for every possible action. The basic workflow now only consists of yarn install, yarn build and yarn test. But you can still run very more precise tasks through yarn test:unit or yarn build:extension, for instance.

Removes the indirection of  the JS files, and runs commands straight
from package.json instead
tsc has been deprecated in favor of typescript
@MaximeKjaer MaximeKjaer requested a review from nicolasstucki July 2, 2020 21:42
The vsix extension should only be built after the changelog commit
Copy link
Contributor

@nicolasstucki nicolasstucki left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

@@ -6,7 +6,7 @@ NEW_VERSION=${TRAVIS_TAG#"v"}

# Build the extension
yarn install
yarn build
yarn build:syntax
Copy link
Contributor

Choose a reason for hiding this comment

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

When is yarn build:extension executed? Is that part of yarn vscode:publish?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is. From the vsce docs it seems that yarn vscode:publish (aka vsce publish) does the packaging before publishing, as it is the only command needed to publish.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok

"ts-node": "^8.10.2",
"tsc": "^1.20150623.0",
"typescript": "^3.9.5",
"vsce": "^1.77.0",
"vscode-tmgrammar-test": "0.0.10"
},
"scripts": {
"vscode:prepublish": "test -f ./syntaxes/Scala.tmLanguage.json",
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 think this prepublish script is actually doing anything useful? I tried messing with the json file but this doesn't result in failures of vscode:prepublish

Copy link
Contributor

Choose a reason for hiding this comment

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

@MaximeKjaer , it just checks if the file ./syntaxes/Scala.tmLanguage.json exists.

@nicolasstucki nicolasstucki merged commit 5b19916 into master Jul 6, 2020
@MaximeKjaer MaximeKjaer deleted the improve-contributing branch July 6, 2020 08:09
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