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

Removes the need for calling require_relative. #241

Closed

Conversation

duggiefresh
Copy link

No description provided.

@translunar
Copy link
Member

This one worries me. I know it says the tests are passing, but I distinctly remember it causing problems when these were moved to the top of the file. I believe it has to do with how the gem works once installed (untestable in specs), but I could be remembering wrong.

@andrewcsmith
Copy link

Right...shouldn't we be doing the whole $:.unshift thing, and then just require 'nmatrix/enumerate, etc.? That seems to be the way most Ruby projects are structured.

@duggiefresh
Copy link
Author

@MohawkJohn Hmm... I'll take a peek at it sometime then.
@andrewcsmith Agreed. Requiring these files can be done in a cleaner manner.

@cjfuller
Copy link
Member

Agreed with @andrewcsmith: I'm a bit confused why these are require_relative at all. The gem puts lib on the loadpath automatically, so this could just be e.g., as he said, require 'nmatrix/enumerate'. Bundler should set up the load path for the dev environment, and gem installation will set it up for users, so the unshift thing shouldn't even be needed if it's done correctly.

@andrewcsmith
Copy link

Right! That's it. Hoe does all this for me now for my own projects...I completely forgot how it works.

Anyway, let's just pull the unshift trick on the load path at the top of our spec helper and call it good.

@translunar
Copy link
Member

Okay. I'll merge a version which uses require instead, but please make sure that you verify that everything still works in an installed version of your updated code.

@translunar
Copy link
Member

@duggiefresh Just wanted to make sure you saw my comment above since I forgot to tag you. Can you verify that it works in an installed version, please?

@duggiefresh
Copy link
Author

@MohawkJohn Sure thing, I'll try and get to it sometime soon. Thanks.

@duggiefresh duggiefresh changed the title Places require_relative declarations at top Removes the need for calling require_relative. Jul 25, 2014
@duggiefresh
Copy link
Author

@MohawkJohn Just updated this PR. Just have a question for you though. When you said:

Just wanted to make sure you saw my comment above since I forgot to tag you. Can you verify that it works in an installed version, please?

Did you mean to verify via running the specs within pkg/nmatrix-0.1.0.rc5? Inside that directory, I am running into an error about not finding nmatrix.so.

@translunar
Copy link
Member

@duggiefresh No. I mean install the version of NMatrix which includes your modifications and check that the stuff that is no longer require_relative'd is still working. You'll have to do it by hand, probably. I don't think the specs are included in the gem manifest.

@translunar
Copy link
Member

@duggiefresh Wanted to ping you and see if you were still interested in getting this merged.

@v0dro
Copy link
Member

v0dro commented Jan 13, 2016

Fixed in latest updates. Closing.

@v0dro v0dro closed this Jan 13, 2016
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

5 participants