Futureproofing #9

Merged
merged 24 commits into from Nov 25, 2015

Conversation

Projects
None yet
2 participants
@BenJWard
Contributor

BenJWard commented Nov 17, 2015

Adding the ability to use different slide creation frameworks with slidewinder.

@BenJWard

This comment has been minimized.

Show comment
Hide comment
@BenJWard

BenJWard Nov 18, 2015

Contributor

This basically works now. You can see I've created a class called PresentationFramework, which contains a method to render the slides, which it does by registering the helpers, and then processing the template. Upon creation it fetches the helper functions, which are stored in the helpers member variable.

Contributor

BenJWard commented Nov 18, 2015

This basically works now. You can see I've created a class called PresentationFramework, which contains a method to render the slides, which it does by registering the helpers, and then processing the template. Upon creation it fetches the helper functions, which are stored in the helpers member variable.

@BenJWard

This comment has been minimized.

Show comment
Hide comment
@BenJWard

BenJWard Nov 18, 2015

Contributor

I'd like to add functionality that allows you to install plugins either with npm or Github, and have slide-winder manage all that stuff for the user. Currently the plugin is just two files in a folder. I guess we should decide how plugins should be bundled up and in what format.

Contributor

BenJWard commented Nov 18, 2015

I'd like to add functionality that allows you to install plugins either with npm or Github, and have slide-winder manage all that stuff for the user. Currently the plugin is just two files in a folder. I guess we should decide how plugins should be bundled up and in what format.

@blahah

This comment has been minimized.

Show comment
Hide comment
@blahah

blahah Nov 19, 2015

Owner

Great work on this @Ward9250, thank you. I'm tied up with another project for a couple of days, but I'll dig in and review the PR as soon as I can. In the meantime just ping me on gitter if I can help answer any questions.

Owner

blahah commented Nov 19, 2015

Great work on this @Ward9250, thank you. I'm tied up with another project for a couple of days, but I'll dig in and review the PR as soon as I can. In the meantime just ping me on gitter if I can help answer any questions.

@blahah blahah self-assigned this Nov 19, 2015

@blahah

This comment has been minimized.

Show comment
Hide comment
@blahah

blahah Nov 19, 2015

Owner

I actually took a quick peek. Looking great! Some comments:

Plugins

I like what you've built - the best thing about it is the simplicity and clarity. I don't think we need to go as far as the npm/Github option in this issue. Let's just build a collection of plugins inside the app, without bundling. Later, we can design a way to have them as external modules without breaking what we have already.

Databases

Storing and managing collections of slides is definitely the idea. However, I'd really like to think carefully about the right way to do that - can you keep anything DB related out of this PR, and we'll discuss it in an issue first? I have various future plans which will affect design decisions on this one, and I'd like to lay that all out.

Tests

We should start using tests. If you want to do that in this PR, that's cool. Or we can wait until this is merged, then setup tests. My favourite spec/test setup in JS is mocha (test framework) + chai (assertion lib), with BDD style, e.g. blah.should.be.a.('string'). See this repo for a minimal example setup.

Coffeescript

The JS generated by coffeescript is .. disgusting! 😝 It would actually be better to not include the generated JS at all - assuming that's an option - and to have it live generated. Having it in the repo makes it seem as though contributors can edit it, which we don't want.

The coffeescript itself is better than the JS, but I think we'll have to follow a strict style guide to keep it accessible and readable. Part of this is that I already juggle 8 languages on a weekly basis, and if I add another one it needs to have minimal surprises. Coffeescript is a lot like ruby (which it is clearly based on), and some of the conveniences it allows can massively decrease readability if one isn't strict.

Can you please follow this style guide?

In addition these rules help maintain accessibility in ruby, and apply equally here I think:

When calling a function/method with more than one argument, wrap the arguments in parenthesis:

some_function arg # good
some_function arg1, arg2 # bad
some_function(arg1, arg2) # good

When nesting function calls use parenthesis to explicitly show the order of operation:

some_function other_function arg1, arg2 # bad bad bad
some_function(other_function(arg1, arg2)) # better

But ideally, don't nest function calls. It only saves lines of code, which are free. It also makes debugging harder. There is sometimes a memory/efficiency cost to creating lots of variables, but this is not a resource intensive program and there's currently no need to optimise. Prefer a single expression per line:

# best
result = other_function(arg1, arg2)
some_function(result)

The only thing I really dislike about coffeescript itself is the indentation-delimited blocks. It really could use an end keyword!

I think the only way to handle this situation is to be very strict about minimising nested indentation. So my final two style requests are:

  1. if you are about to nest something more than one level within a function/method, break it out into a new function/method
  2. bring the indentation back to the starting level at the end of each block, even if it means redundantly naming a return value
# bad
plan_day = (day) ->
  switch day
    when "Mon" then go work
    when "Tue" then go relax
    when "Thu" then go iceFishing
    when "Fri", "Sat"
      if day is bingoDay
        go bingo
        go dancing

# good (two functions, each pulls back to starting indentation)
plan_day = (day) ->
  switch day
    when "Mon" then go work
    when "Tue" then go relax
    when "Thu" then go iceFishing
    when "Fri", "Sat" then bingo_or_dancing day
  null

bingo_or_dancing = (day) ->
  if day is bingoDay
    go bingo
  else go dancing
Owner

blahah commented Nov 19, 2015

I actually took a quick peek. Looking great! Some comments:

Plugins

I like what you've built - the best thing about it is the simplicity and clarity. I don't think we need to go as far as the npm/Github option in this issue. Let's just build a collection of plugins inside the app, without bundling. Later, we can design a way to have them as external modules without breaking what we have already.

Databases

Storing and managing collections of slides is definitely the idea. However, I'd really like to think carefully about the right way to do that - can you keep anything DB related out of this PR, and we'll discuss it in an issue first? I have various future plans which will affect design decisions on this one, and I'd like to lay that all out.

Tests

We should start using tests. If you want to do that in this PR, that's cool. Or we can wait until this is merged, then setup tests. My favourite spec/test setup in JS is mocha (test framework) + chai (assertion lib), with BDD style, e.g. blah.should.be.a.('string'). See this repo for a minimal example setup.

Coffeescript

The JS generated by coffeescript is .. disgusting! 😝 It would actually be better to not include the generated JS at all - assuming that's an option - and to have it live generated. Having it in the repo makes it seem as though contributors can edit it, which we don't want.

The coffeescript itself is better than the JS, but I think we'll have to follow a strict style guide to keep it accessible and readable. Part of this is that I already juggle 8 languages on a weekly basis, and if I add another one it needs to have minimal surprises. Coffeescript is a lot like ruby (which it is clearly based on), and some of the conveniences it allows can massively decrease readability if one isn't strict.

Can you please follow this style guide?

In addition these rules help maintain accessibility in ruby, and apply equally here I think:

When calling a function/method with more than one argument, wrap the arguments in parenthesis:

some_function arg # good
some_function arg1, arg2 # bad
some_function(arg1, arg2) # good

When nesting function calls use parenthesis to explicitly show the order of operation:

some_function other_function arg1, arg2 # bad bad bad
some_function(other_function(arg1, arg2)) # better

But ideally, don't nest function calls. It only saves lines of code, which are free. It also makes debugging harder. There is sometimes a memory/efficiency cost to creating lots of variables, but this is not a resource intensive program and there's currently no need to optimise. Prefer a single expression per line:

# best
result = other_function(arg1, arg2)
some_function(result)

The only thing I really dislike about coffeescript itself is the indentation-delimited blocks. It really could use an end keyword!

I think the only way to handle this situation is to be very strict about minimising nested indentation. So my final two style requests are:

  1. if you are about to nest something more than one level within a function/method, break it out into a new function/method
  2. bring the indentation back to the starting level at the end of each block, even if it means redundantly naming a return value
# bad
plan_day = (day) ->
  switch day
    when "Mon" then go work
    when "Tue" then go relax
    when "Thu" then go iceFishing
    when "Fri", "Sat"
      if day is bingoDay
        go bingo
        go dancing

# good (two functions, each pulls back to starting indentation)
plan_day = (day) ->
  switch day
    when "Mon" then go work
    when "Tue" then go relax
    when "Thu" then go iceFishing
    when "Fri", "Sat" then bingo_or_dancing day
  null

bingo_or_dancing = (day) ->
  if day is bingoDay
    go bingo
  else go dancing
@BenJWard

This comment has been minimized.

Show comment
Hide comment
@BenJWard

BenJWard Nov 19, 2015

Contributor

@blahah, thanks for the feedback. I thought nedb would be a good 'database' to use, it actually just makes easy through a database like api, storing persistent data to files in json format, which is what I was going to write manually anyway. The main justification for this was you could do something like slidewinder collection add ~/Folderwithslidesinside and it would add to the db the slides, but also any image files or other resources used by said slides, as people forget them sometimes! It also means you parse your slides once, and then they are in there, and you could even make sharing slides (this is node so I thought being async so as things could be web-ified if desired might be good) easy as you just have to extract and send elements of the "database". I was hesitant to go for a full blow db like mongo or anything like that, because I thought it was - right now - overkill. This also meant installing plugins/frameworks and managing them - as it's just some functions, and some html - which can be stored as json.

If you want to make the Issue of the design goals you had, and if necessary, if any of this is too much, I can roll back this to before I started the content management stuff and just add the ability to choose a framework, then I'll correct the style.

Contributor

BenJWard commented Nov 19, 2015

@blahah, thanks for the feedback. I thought nedb would be a good 'database' to use, it actually just makes easy through a database like api, storing persistent data to files in json format, which is what I was going to write manually anyway. The main justification for this was you could do something like slidewinder collection add ~/Folderwithslidesinside and it would add to the db the slides, but also any image files or other resources used by said slides, as people forget them sometimes! It also means you parse your slides once, and then they are in there, and you could even make sharing slides (this is node so I thought being async so as things could be web-ified if desired might be good) easy as you just have to extract and send elements of the "database". I was hesitant to go for a full blow db like mongo or anything like that, because I thought it was - right now - overkill. This also meant installing plugins/frameworks and managing them - as it's just some functions, and some html - which can be stored as json.

If you want to make the Issue of the design goals you had, and if necessary, if any of this is too much, I can roll back this to before I started the content management stuff and just add the ability to choose a framework, then I'll correct the style.

@BenJWard

This comment has been minimized.

Show comment
Hide comment
@BenJWard

BenJWard Nov 19, 2015

Contributor

As a note, I wonder if you could look specifically at the latest commit, at the parse_md_slides option. This process uses several async reading operations with callbacks and so because you want to do things to the parsed slides usually, I gave the whole function a callback parameter that accepts 'slides'. This seemed a more node-ey way to do it, you almost always see people say don't do sync, when somebody asks how to.

I was thinking, is it appropriate to split the slidewinder repo (time for a new org?) with repo's for the library of specific functionality, the cmdnline app, and so on?

Contributor

BenJWard commented Nov 19, 2015

As a note, I wonder if you could look specifically at the latest commit, at the parse_md_slides option. This process uses several async reading operations with callbacks and so because you want to do things to the parsed slides usually, I gave the whole function a callback parameter that accepts 'slides'. This seemed a more node-ey way to do it, you almost always see people say don't do sync, when somebody asks how to.

I was thinking, is it appropriate to split the slidewinder repo (time for a new org?) with repo's for the library of specific functionality, the cmdnline app, and so on?

@BenJWard

This comment has been minimized.

Show comment
Hide comment
@BenJWard

BenJWard Nov 19, 2015

Contributor

Ok, I've just moved this branch back to the point where the plugin thing worked, before I started thinking about persistent data and content management - which would in my mind also alter plugin system, like you'd install slides, you'd install plugins. But for now this works.

Contributor

BenJWard commented Nov 19, 2015

Ok, I've just moved this branch back to the point where the plugin thing worked, before I started thinking about persistent data and content management - which would in my mind also alter plugin system, like you'd install slides, you'd install plugins. But for now this works.

@blahah

This comment has been minimized.

Show comment
Hide comment
@blahah

blahah Nov 21, 2015

Owner

Just briefly on the question about a new org - I don't think we need separate repos for the library and cmdline app. In general I write tools so that the CLI and the library are the same package, but with two ways to get at the same interface. However, if you're interested in joining slidewinder as a co-lead (i.e., we start a team to work on it), then I'll happily create an org so that we both get equal credit, rather than it being associated solely with my name. I'll make a separate issue with the general design goals - or perhaps write out my previous ideas for the roadmap on the wiki.

Last but most important, I'll review the code in the PR tomorrow. Thanks again for this.

Owner

blahah commented Nov 21, 2015

Just briefly on the question about a new org - I don't think we need separate repos for the library and cmdline app. In general I write tools so that the CLI and the library are the same package, but with two ways to get at the same interface. However, if you're interested in joining slidewinder as a co-lead (i.e., we start a team to work on it), then I'll happily create an org so that we both get equal credit, rather than it being associated solely with my name. I'll make a separate issue with the general design goals - or perhaps write out my previous ideas for the roadmap on the wiki.

Last but most important, I'll review the code in the PR tomorrow. Thanks again for this.

@blahah

This comment has been minimized.

Show comment
Hide comment
@blahah

blahah Nov 22, 2015

Owner

See https://github.com/slidewinder - we'll move there once this PR is merged. :)

Owner

blahah commented Nov 22, 2015

See https://github.com/slidewinder - we'll move there once this PR is merged. :)

@BenJWard

This comment has been minimized.

Show comment
Hide comment
@BenJWard

BenJWard Nov 22, 2015

Contributor

I've just implemented the style suggested.

Contributor

BenJWard commented Nov 22, 2015

I've just implemented the style suggested.

@blahah

This comment has been minimized.

Show comment
Hide comment
@blahah

blahah Nov 22, 2015

Owner

Looks much better now :). Final thing on style is the 2-space indent. I've commited a .editorconfig to the master branch that enforces/describes the spacing.

I just pulled and ran the code in this branch - mostly looks good, but a couple of comments:

  1. A test command (bin/cmdline.js --collection examples --slides code,data,list --title test --author me --output test --framework remark) creates a slideshow, but the metadata slide doesn't actually fill the metadata templates
  2. Ideally the CLI will list the available frameworks. If there are fewer than 5, they should be shown in the help message, otherwise there should be a new command --list-frameworks.

I'll dig into the code shortly.

Owner

blahah commented Nov 22, 2015

Looks much better now :). Final thing on style is the 2-space indent. I've commited a .editorconfig to the master branch that enforces/describes the spacing.

I just pulled and ran the code in this branch - mostly looks good, but a couple of comments:

  1. A test command (bin/cmdline.js --collection examples --slides code,data,list --title test --author me --output test --framework remark) creates a slideshow, but the metadata slide doesn't actually fill the metadata templates
  2. Ideally the CLI will list the available frameworks. If there are fewer than 5, they should be shown in the help message, otherwise there should be a new command --list-frameworks.

I'll dig into the code shortly.

@BenJWard

This comment has been minimized.

Show comment
Hide comment
@BenJWard

BenJWard Nov 22, 2015

Contributor

With command 1, I wonder this might be because the remark plugin is missing something? Perhaps I accidentally removed when I seperated the remark processing into it's seperate framework section.

Contributor

BenJWard commented Nov 22, 2015

With command 1, I wonder this might be because the remark plugin is missing something? Perhaps I accidentally removed when I seperated the remark processing into it's seperate framework section.

@BenJWard

This comment has been minimized.

Show comment
Hide comment
@BenJWard

BenJWard Nov 22, 2015

Contributor

Just solved the issue with metadata not appearing in the slides. I'm implementing a fix now.

Contributor

BenJWard commented Nov 22, 2015

Just solved the issue with metadata not appearing in the slides. I'm implementing a fix now.

@blahah

This comment has been minimized.

Show comment
Hide comment
@blahah

blahah Nov 23, 2015

Owner

Excellent :)

Owner

blahah commented Nov 23, 2015

Excellent :)

@BenJWard

This comment has been minimized.

Show comment
Hide comment
@BenJWard

BenJWard Nov 23, 2015

Contributor

The issue was indeed with the plugin, you'll see I've added lines that add the attributes of the entire deck to the attributes of each slide. The attributes of each slide are then added to the body of the slides, and then finally the entire combo is pasted into the template. Before only the slide body was pasted. So there was no slide metadata. This was largely because I didn't understand, we use handlebars to edit our html template, BUT the handlebars in the slides, are not processed then, they are added as plaintext into the result of the HTML, then on opening the slides, remark.jl is actually what processes those handlebars according to the front matter added to each slide.

Contributor

BenJWard commented Nov 23, 2015

The issue was indeed with the plugin, you'll see I've added lines that add the attributes of the entire deck to the attributes of each slide. The attributes of each slide are then added to the body of the slides, and then finally the entire combo is pasted into the template. Before only the slide body was pasted. So there was no slide metadata. This was largely because I didn't understand, we use handlebars to edit our html template, BUT the handlebars in the slides, are not processed then, they are added as plaintext into the result of the HTML, then on opening the slides, remark.jl is actually what processes those handlebars according to the front matter added to each slide.

@BenJWard

This comment has been minimized.

Show comment
Hide comment
@BenJWard

BenJWard Nov 23, 2015

Contributor

This raises questions: remark jl clearly processes handlebars, .md, and supports front-matter for indiviudal slides, which are delimited by ---, but other frameworks like impress.js don't support this, or do in a diferent way. I don't think it supports metadata and handlebar processing, and is generally written in html. But obviously we want to be able to use all our slides, from all the frameworks. So clearly the remark.js plugin is easy, and just sorts out the text to paste in the template, but other plugins needs a few more things: I propose the framework class should be able to contain any number of slide preprocessing functions, which are used after the loading and selection of slides, but before the render stage already implemented in the class. The class will be able to load these in, dynamically from the chosen framework, just like it currently does for the rendering helpers. For the impress.js plugin, this might include processing slide bodies for handlebars and metadata, the automatic generation of attributes not provided in the slides, but that are required (impress.js requires a user to decide on x, y, and z positioning - although I've seen utilities that auto-decide on this) and so on.

Contributor

BenJWard commented Nov 23, 2015

This raises questions: remark jl clearly processes handlebars, .md, and supports front-matter for indiviudal slides, which are delimited by ---, but other frameworks like impress.js don't support this, or do in a diferent way. I don't think it supports metadata and handlebar processing, and is generally written in html. But obviously we want to be able to use all our slides, from all the frameworks. So clearly the remark.js plugin is easy, and just sorts out the text to paste in the template, but other plugins needs a few more things: I propose the framework class should be able to contain any number of slide preprocessing functions, which are used after the loading and selection of slides, but before the render stage already implemented in the class. The class will be able to load these in, dynamically from the chosen framework, just like it currently does for the rendering helpers. For the impress.js plugin, this might include processing slide bodies for handlebars and metadata, the automatic generation of attributes not provided in the slides, but that are required (impress.js requires a user to decide on x, y, and z positioning - although I've seen utilities that auto-decide on this) and so on.

@blahah

This comment has been minimized.

Show comment
Hide comment
@blahah

blahah Nov 25, 2015

Owner

I completely agree about the multiple methods for a framework plugin. We may also need to add hooks into the different methods of slidewinder so that it will call the framework plugin at the correct point if necessary. But for now, I'll merge what we've got, and we can extend as necessary when a new framework is added. We don't need to be completely future-proof right now - being functional and extensible is a great place to be :)

Thanks very much for this PR, it has massively improved slidewinder.

Owner

blahah commented Nov 25, 2015

I completely agree about the multiple methods for a framework plugin. We may also need to add hooks into the different methods of slidewinder so that it will call the framework plugin at the correct point if necessary. But for now, I'll merge what we've got, and we can extend as necessary when a new framework is added. We don't need to be completely future-proof right now - being functional and extensible is a great place to be :)

Thanks very much for this PR, it has massively improved slidewinder.

blahah added a commit that referenced this pull request Nov 25, 2015

Merge pull request #9 from Ward9250/futureproofing
Futureproofing
- coffeescript
- framework plugin system

@blahah blahah merged commit 5390af9 into blahah:master Nov 25, 2015

@BenJWard

This comment has been minimized.

Show comment
Hide comment
@BenJWard

BenJWard Nov 25, 2015

Contributor

Indeed, it is in a useable state now, I'll probably use it for the Cambridge talk, which I'll be adding to BioJulia/talks as I do it.

Contributor

BenJWard commented Nov 25, 2015

Indeed, it is in a useable state now, I'll probably use it for the Cambridge talk, which I'll be adding to BioJulia/talks as I do it.

@blahah

This comment has been minimized.

Show comment
Hide comment
@blahah

blahah Nov 25, 2015

Owner

great :D

Owner

blahah commented Nov 25, 2015

great :D

@blahah

This comment has been minimized.

Show comment
Hide comment
@blahah

blahah Nov 25, 2015

Owner

cool - I'll use it for my opencon talk tomorrow

Owner

blahah commented Nov 25, 2015

cool - I'll use it for my opencon talk tomorrow

@blahah blahah referenced this pull request in slidewinder/slidewinder Jan 11, 2016

Closed

Futureproofing #9

@blahah

This comment has been minimized.

Show comment
Hide comment
@blahah

blahah Jan 11, 2016

Owner

This issue was moved to slidewinder/slidewinder#9

Owner

blahah commented Jan 11, 2016

This issue was moved to slidewinder/slidewinder#9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment