Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

Misc. improvements #717

Merged
merged 4 commits into from
Feb 28, 2015
Merged

Misc. improvements #717

merged 4 commits into from
Feb 28, 2015

Conversation

am11
Copy link
Contributor

@am11 am11 commented Feb 27, 2015



  • Moves pangyp from dev to main dependencies in
    package.json.

The new version text looks like:

$ node-sass -v

node-sass 2.0.1  (Wrapper)    [JavaScript]
libsass   3.1.0  (Sass Compiler)   [C/C++]

@lukeapage, hope you don't mind! ©️ 😉


  • Repo: Updates CONTRIBUTING.md.
    • Adds instructions.
    • Removes obsolete info.

This is for DRY'ing comments on repeating issues.


  • CI: Updates and formats configuration files.

am11 added a commit to am11/node-sass that referenced this pull request Feb 27, 2015
* Remove tests from publishing. sass#683
  * Uses new strategy to validate binary after
    the download.
  * Moves mocha under dev-dependencies in
    package.json.
* Truncates the forth potion from v8 version
  as discussed in sass#710.
* Moves pangyp from dev to main dependencies in
  package.json.

Issue URLs: sass#683 and sass#710.
PR URL: sass#717.
am11 added a commit to am11/node-sass that referenced this pull request Feb 27, 2015
* Removes tests from publishing. sass#683
  * Uses new strategy to validate binary after
    the download.
  * Moves mocha under dev-dependencies in
    package.json.
* Truncates the forth portion from v8 version
  as discussed in sass#710.
* Moves pangyp from dev to main dependencies in
  package.json.

Issue URLs: sass#683 and sass#710.
PR URL: sass#717.
@am11
Copy link
Contributor Author

am11 commented Feb 27, 2015

@andreitognolo, does SnapCI actually build and run the tests? It never fails (even when we have something broken) and reports back success in about five seconds! Are we missing something here?

lib-cov
node_modules
vendor
test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is .gitignore union test, because npm docs suggest: in absence of .npmignore, it takes .gitignore into account and when .npmignore is present, it essentially overrides .gitignore.

am11 added a commit to am11/node-sass that referenced this pull request Feb 28, 2015
* Based on sindresorhus/meow#2.
* Improves info text; inspired by Less CLI. :)

PR URL: sass#717.
am11 added a commit to am11/node-sass that referenced this pull request Feb 28, 2015
* Based on sindresorhus/meow#2.
* Adds alias -v.
* Improves info text; inspired by Less CLI. :)

PR URL: sass#717.
@am11 am11 changed the title Build: Misc. improvements Misc. improvements Feb 28, 2015
am11 added a commit to am11/node-sass that referenced this pull request Feb 28, 2015
* Based on sindresorhus/meow#2.
* Adds alias -v.
* Improves info text; inspired by Less CLI. :)

PR URL: sass#717.
@@ -28,10 +30,26 @@ function getRuntimeInfo() {
*/

function getBinaryIdentifiableName() {
var v8SemVersion = process.versions.v8;
Copy link
Contributor

Choose a reason for hiding this comment

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

All this code is essentially just

var v8SemVersion = process.versions.v8.split('.').slice(0, 2).join('.')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't it be .slice(0, 3) to extract semver?

In some releases (for instance node v0.12.0), there are three portions (3.28.73). While in others (all iojs releases till date) have four portions.

I thought it would be good to save a join when v8 version already have three portions.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're correct it should be (0,3)

I thought it would be good to save a join when v8 version already have three portions.

Since this is essentially a one off operation I don't see any benefit in saving a join when it comes at the cost of added verbosity.

It took me a couple reads of this this code to understand it and I already knew what it was trying to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. 👍

Fixed by 9bd034c.

am11 added a commit to am11/node-sass that referenced this pull request Feb 28, 2015
* Removes tests from publishing. sass#683
  * Uses new strategy to validate binary after
    the download.
  * Moves mocha under dev-dependencies in
    package.json.
* Truncates the forth portion from v8 version
  as discussed in sass#710.
* Moves pangyp from dev to main dependencies in
  package.json.

Issue URLs: sass#683 and sass#710.
PR URL: sass#717.
am11 added a commit to am11/node-sass that referenced this pull request Feb 28, 2015
* Based on sindresorhus/meow#2.
* Adds alias -v.
* Improves info text; inspired by Less CLI. :)

PR URL: sass#717.
am11 added a commit to am11/node-sass that referenced this pull request Feb 28, 2015
* Based on sindresorhus/meow#2.
* Adds alias -v.
* Improves info text; inspired by Less CLI. :)

PR URL: sass#717.
am11 added a commit to am11/node-sass that referenced this pull request Feb 28, 2015
* Based on sindresorhus/meow#2.
* Adds alias -v.
* Improves info text; inspired by Less CLI. :)
* Updates readme.

PR URL: sass#717.
* Removes tests from publishing. sass#683
  * Uses new strategy to validate binary after
    the download.
  * Moves mocha under dev-dependencies in
    package.json.
* Truncates the forth portion from v8 version
  as discussed in sass#710.
* Moves pangyp from dev to main dependencies in
  package.json.

Issue URLs: sass#683 and sass#710.
PR URL: sass#717.
* Based on sindresorhus/meow#2.
* Adds alias -v.
* Improves info text; inspired by Less CLI. :)
* Updates readme.

PR URL: sass#717.
* Adds instructions.
* Removes obsolete info.

This is for DRY'ing comments on repeating issues.
@am11 am11 removed the Dev - WIP label Feb 28, 2015
@am11 am11 force-pushed the build-fixes branch 4 times, most recently from 23eeed8 to 3d4042e Compare February 28, 2015 07:01
am11 added a commit that referenced this pull request Feb 28, 2015
@am11 am11 merged commit 6a10b92 into sass:master Feb 28, 2015
@am11 am11 deleted the build-fixes branch February 28, 2015 07:12
@am11 am11 removed the up-for-grabs label Feb 28, 2015
jiongle1 pushed a commit to scantist-ossops-m2/node-sass that referenced this pull request Apr 7, 2024
Implement emitting utf8 charset when seen in output
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants