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

Update generated index.coffee to use fs.existsSync #19

Merged
merged 1 commit into from
Dec 22, 2014

Conversation

lilyball
Copy link
Contributor

Script loading should be synchronous, to preserve script load order and
allow the script to behave as expected with the --config-check hubot
flag. Also ensure the scripts are loaded in alphabetical order. This
matches how hubot loads scripts from the scripts/ directory.

Script loading should be synchronous, to preserve script load order and
allow the script to behave as expected with the `--config-check` hubot
flag. Also ensure the scripts are loaded in alphabetical order. This
matches how hubot loads scripts from the scripts/ directory.
@technicalpickles
Copy link
Member

Thinking about this a little bit, and one nice thing about the change you proposed to the docs in hubotio/hubot#831 with just calling robot.load is that there's a ton less magic boilerplate code.

I think the only 'weird' thing about making the index.coffee try to load everything in src is that it means it expects everything in that directory a script, which means you can't as easily do more object oriented type stuff splitting into their own files.

I'm thinking to perhaps replace this with like:

path = require 'path'

module.exports = (robot, scripts) ->
  scriptsPath = path.resolve(__dirname, 'src')
  robot.loadFile(scriptsPath, "hardcoded.coffee")

Because this is a generator, and we are creating the file in src, we can generate this hardcoded string.

@lilyball
Copy link
Contributor Author

@technicalpickles Generating code like that makes it hard to then add extra src files in the future, if you expect them to load automatically by being present (which is how the existing code works). I guess it depends on what you expect to happen when adding a new file to the src directory.

@technicalpickles
Copy link
Member

Things are weird one way or the other. Either it assumes that everything in src/ is a script and tries to load it, meaning you can abstract code to other files in the directory... or, it doesn't automatically pick up files in src, requiring you to add code in `index.coffee, but doesn't make any assumptions about files in that directory.

Another thing that had bugged me was that it is a bit boiler plately, and I was having trouble parsing why it's going through reading the directory, and conditionally loading files. Then I remembered why: https://github.com/github/hubot/blob/master/src/robot.coffee#L252-L266 . Basically, it lets you have external-scripts.json as a object, where you can list out which files to load or '*' for all of them.

@technicalpickles
Copy link
Member

So, I think it's probably best to go with this way for now just to get the sync directory reading, and can reconsider changing the default later on in #20

@lilyball
Copy link
Contributor Author

Yeah, I think the conditional load stuff makes more sense if your package contains a scripts directory and therefore is just a collection of disparate scripts. But most packages are likely to be a single unit that doesn't actually want the conditional loading at all. Given that, I'm in favor of the change for #20, though as you say, better to switch it to sync now if #20 is going to take time to consider.

technicalpickles added a commit that referenced this pull request Dec 22, 2014
Update generated index.coffee to use fs.existsSync
@technicalpickles technicalpickles merged commit 4a98e43 into hubotio:master Dec 22, 2014
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.

None yet

2 participants