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

Running find_and_require_user_defined_code on require is wrong #68

Closed
joerixaop opened this issue Nov 21, 2012 · 6 comments
Closed

Running find_and_require_user_defined_code on require is wrong #68

joerixaop opened this issue Nov 21, 2012 · 6 comments

Comments

@joerixaop
Copy link

Wrong is perhaps a harsh word, but I don't think a library should be requiring user defined code at all, let alone automatically.

This caused a problem when I deployed something in production and monitored it with monit. Suddenly the lack of a HOME in the default monit environment prevents the application from running.

If one of my coworkers would start using this helpful method to load helper code he might start relying on it. Code would break on other people's machines (I know, unit tests, but still when that coworker would be running tests he wouldn't encounter the issue, muddling the source of the errors).

Worse, I have no way to prevent this loading of untested code from outside my application. The only way to avoid loading code is by specifying a foldername to ENV["neography_extensions"] that does not exist and then hope that no-one is ever going to create it (either on purpose or by accident).

The only useful role that I see for something like this is when working in an interactive environment, in that case it might be better to have either a separate require file that would do a normal require of neography.rb and then load the "extensions" separately.

Perhaps the worst part of this feature is that it's not documented at all (as far as I can see). So only people who read the code of all libraries they require will even know about it until they encounter an issue like I did with monit.

@joerixaop
Copy link
Author

Oh, and I'm willing to write a patch for this, but I don't know how you would like this to work.
options: default off (env flag/different require), default on (idem) or completely removed

@rdvdijk
Copy link
Collaborator

rdvdijk commented Nov 21, 2012

Personally, I wouldn't mind seeing this removed entirely. @maxdemarzi, what do you think?

@maxdemarzi
Copy link
Owner

Let's add something for calling unmanaged extensions, and rip this out.

neo.unmanaged("/extensions/dosomethingcool")

@joerixaop
Copy link
Author

It still is a bit unclear to me why that would be the concern of the neography gem? There is nothing specific to neography in the code, all it does is load all ruby files in a folder (but not in nested folders).

so neo.unmanaged("/extensions/dosomethingcool") can be written as Dir["/extensions/dosomethingcool/*.rb"].each {|file| require file }
A bit longer, but not by that much. Also not that difficult to remember if you do it a few times. And if it's used mainly from irb or the rails console it can be added to .irbrc

If a user really needs to load a full directory then there is a gem for that too: https://github.com/jarmo/require_all

@rdvdijk
Copy link
Collaborator

rdvdijk commented Nov 23, 2012

@maxdemarzi , I have to agree with @joerixaop here. I feel this does not really add value to the gem, any custom code of the user should just go into the application that is using neography. What was the original idea of adding this?

@maxdemarzi
Copy link
Owner

Let's go ahead and remove it.

willkessler pushed a commit to willkessler/neography that referenced this issue Apr 21, 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

No branches or pull requests

3 participants