Skip to content
This repository has been archived by the owner on Oct 21, 2022. It is now read-only.

fix #20 - add a workaround for undefined export var #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rbecheras
Copy link

@rbecheras rbecheras commented Nov 28, 2016

@@ -93,10 +93,12 @@ function sbRequire(require, resolve, path) {
if(!getSB(path)) {
if(path.match(/[\\\/]/) && fpath.extname(path) == ".js") {
var sb = sandboxes[path.toLowerCase()] = context(path);
Copy link
Author

@rbecheras rbecheras Nov 29, 2016

Choose a reason for hiding this comment

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

I don't think so... But its coding style, so it's about conventions so my opinion doesn't matter. at all.

If the maintainers say to do this that way, sure I will.

But if you take a look at this project coding style, and especially for this file, it seems it's my way that is applied.

var util = require("util");

Copy link
Contributor

Choose a reason for hiding this comment

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

For what it is worth, I am not aware of anyone actively maintaining this plugin. So, unless another Light Table maintainer says otherwise, I'll leave it up to you.

Copy link
Author

Choose a reason for hiding this comment

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

Ok so, who is able to merge the PRs ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, to clarify, anyone on the Light Table team can review and merge it.

My comment was simply to suggest it is not likely that any of us will tell you to change the coding style.

Copy link
Author

@rbecheras rbecheras Nov 30, 2016

Choose a reason for hiding this comment

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

@sbauer322 Ok. Thanks.
And so.. Do you need a maintainer for this repo? I would be happy to join you as maintainer for the JavaScript plugin or even for the main or the other side projects. I search to invest myself in OSS joining maintainer teams and GitHub organizations for the projects I love. And Light Table is becoming one of them ;-)

Copy link
Author

Choose a reason for hiding this comment

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

@Mouvedia If you create an issue for your feature request, mention me with the arobase symbol, so that I'll get notified

Copy link
Contributor

@sbauer322 sbauer322 Nov 30, 2016

Choose a reason for hiding this comment

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

A number of people desire a better JavaScript experience with Light Table and I think it would be great to have an active maintainer for this plugin (and other JS related ones). You also seem to have a long history of contributing to open source, which is beneficial.

My main concern is if you are familiar, or at least willing to learn, Clojure or ClojureScript? Being an expert in them wouldn't be necessary, but at least enough to deal with the plugin's code. Both Light Table and some of this plugin are written in them. Aside from this, I don't have any qualms... @rundis , @cldwalker - any concerns or thoughts?

This pull request is likely not the proper location to have this discussion, but perhaps our Gitter would be better.

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@rbecheras - what would the simplest way to reproduce the error to make sure this PR fixes it? This linked issue, LightTable/LightTable#2146, gave some steps... should that suffice?

Copy link

Choose a reason for hiding this comment

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

大哥能帮我翻译一下评论吗😉😉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants