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

Implement #3332 #3333

Merged
merged 7 commits into from
Jan 28, 2014
Merged

Implement #3332 #3333

merged 7 commits into from
Jan 28, 2014

Conversation

xixixao
Copy link
Contributor

@xixixao xixixao commented Jan 28, 2014

I have intentionally split this up into separate commits, the first one is the actual change from Ruby to Node. Having the whole _.template function in Cakefile is not ideal (I cut it down though), but at least doesn't add another dependency. Better solutions welcome.

I've done this after three pints, so peer review would be highly welcomed.

Little note: Highlight.js parses object keys as attribute, so I deleted it from the css file to keep an equivalent style.

Second note: @michaelficarra the run expressions are javascript expressions, so we can't use #{}

@michaelficarra
Copy link
Collaborator

Whoops, sorry about that!

@epidemian
Copy link
Contributor

Hehe, i knew you'd probably be working on this. At least i hadn't done much to merge #3198 into master yet 😅

@xixixao
Copy link
Contributor Author

xixixao commented Jan 28, 2014

@epidemian Well, something like that was expected. Let me have a look and merge the two together?

@@ -12,7 +12,7 @@ helpers = require './helpers'
SourceMap = require './sourcemap'

# The current CoffeeScript version number.
exports.VERSION = '1.6.3'
exports.VERSION = '1.7.0'
Copy link
Collaborator

Choose a reason for hiding this comment

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

The only reason this is hardcoded is because of the web version, right ?

@jashkenas
Copy link
Owner

Having the whole _.template function in Cakefile is not ideal (I cut it down though), but at least doesn't add another dependency.

Instead of doing that, just put Underscore as a devDependency, like the others — dev dependencies don't really matter.

@xixixao
Copy link
Contributor Author

xixixao commented Jan 28, 2014

@jashkenas I'll use echo as in the mentioned commit, underscore doesn't support CS out of the box. It is quite lightweight in terms of dependencies. The only thing is that npm doesn't install coffee-script to its npm-modules (I assume because we are writing coffee-script), but then Node can't find it. I'll look into it later.

@jashkenas
Copy link
Owner

Oh, you mean eco? Sure, that's fine too.

The only thing is that npm doesn't install coffee-script to its npm-modules (I assume because we are writing coffee-script), but then Node can't find it.

We can probably assume that you have a version of coffee exposed globally if you're going ahead and rebuilding the docs, so this should be fine.

@xixixao
Copy link
Contributor Author

xixixao commented Jan 28, 2014

eco kinda sucks, but oh well... sstephenson/eco#41

@michaelficarra
Copy link
Collaborator

I'd prefer to stay away from a dependency on eco if at all possible. We don't want to ditch one dead technology in favour of another dead technology.

@bjmiller
Copy link

I vote for keeping it simple. Failing that, if you're not going to do it with underscore templates, then your best bet is probably ECT. CoffeeKup would be awesome, but it's also not getting much maintenance love.

@vendethiel
Copy link
Collaborator

Eco is dead (last commit 2 years ago), EJS is dead (last commit in 2009), damn.
I guess there's toffee that's maintained ... But really, it doesn't matter

@xixixao
Copy link
Contributor Author

xixixao commented Jan 28, 2014

@michaelficarra I'd say this is harmless, eco will work as long as npm keeps its version in the registry. Worse thing is: npm/npm#1341, so you have to manually run npm install inside eco's directory. Global install doesn't help.

@xixixao
Copy link
Contributor Author

xixixao commented Jan 28, 2014

@Nami-Doc ok, I'll try toffee, at least it's got multiline code blocks. Everything's gonna have the npm install problem though.

@bjmiller
Copy link

Spent a few minutes fork-jumping. Looks like this is under current maintenance: https://github.com/goodeggs/teacup

@jashkenas
Copy link
Owner

ok, I'll try toffee, at least it's got multiline code blocks.

Or, instead, you could just stick to Underscore templates (which are about the simplest possible thing for this use case). Having a few lines of JavaScript instead of CoffeeScript at the top of the file is no big deal. And/or you can pull more of the data configuration over into the Cakefile.

@vendethiel
Copy link
Collaborator

Yeah, seems like my last edit was not submitted. Let's just use the simplest option, underscore templates

@xixixao
Copy link
Contributor Author

xixixao commented Jan 28, 2014

Underscore it is, toffee is troublesome, codeFor will go to Cakefile and codeFor calls with be in JS.

  • turns out my original wasn't such a bad solution

@xixixao
Copy link
Contributor Author

xixixao commented Jan 28, 2014

Done?

@epidemian
Copy link
Contributor

LGTM 👍

@jashkenas
Copy link
Owner

Almost there. Maybe only require highlightJS when you're going to run the doc:site task?

@xixixao
Copy link
Contributor Author

xixixao commented Jan 28, 2014

@jashkenas Done

jashkenas added a commit that referenced this pull request Jan 28, 2014
@jashkenas jashkenas merged commit 4689541 into jashkenas:master Jan 28, 2014
@tiye
Copy link

tiye commented Jan 29, 2014

@Nami-Doc I thought last commit of ejs is "2 months ago"?
https://github.com/visionmedia/ejs/commits/master

@vendethiel
Copy link
Collaborator

@jiyinyiyong oh right, googling it got me on [http://embeddedjs.com/](this website)

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.

7 participants