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

Fix "Already initialized constant" warnings when running #298

Closed

Conversation

stanhu
Copy link
Contributor

@stanhu stanhu commented Jul 31, 2015

This builds upon https://github.com/rumpelsepp/rugments/commit/e6bdd4eeed47670710d48f53c4b7759a35f1f348, but ensues the specs and rackup app work.

Closes #296

@rumpelsepp
Copy link
Contributor

There are more load things I think: #291

@stanhu
Copy link
Contributor Author

stanhu commented Jul 31, 2015

@rumpelsepp Thanks, I updated the pull request. There are also more load_const calls as well. I'm not sure if #291 works with rackup, though. Manually cleaning up $LOADED_FEATUES was required.

@rodrigoargumedo
Copy link

This will also need to close #290. This seems to be good. Seeking a merge on this one.

Closes #290

@stanhu
Copy link
Contributor Author

stanhu commented Aug 2, 2015

@jneen, any thoughts?

@@ -2,6 +2,15 @@

# stdlib
require 'pathname'
require_relative 'rouge/version'
Copy link
Contributor

Choose a reason for hiding this comment

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

require_relative isn't available on 1.8.7

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given the .travis.yml seems to test a minimum version of Ruby v1.9.3, I don't think this should be a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

But why require_relative over require?

Copy link

Choose a reason for hiding this comment

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

Maybe the intent is more clear with require_relative

On 15 aug 2015, at 20:21, Josh Cheek [email protected] wrote:

In lib/rouge.rb:

@@ -2,6 +2,15 @@

stdlib

require 'pathname'
+require_relative 'rouge/version'
But why not just use require?


Reply to this email directly or view it on GitHub.

Choose a reason for hiding this comment

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

I think require_relative is okay, for now. @jneen what are your thoughts on this?

On Aug 15, 2015, at 5:56 PM, "Patrik Ragnarsson" [email protected] wrote:

In lib/rouge.rb:

@@ -2,6 +2,15 @@

stdlib

require 'pathname'
+require_relative 'rouge/version'
Maybe the intent is more clear with require_relative


Reply to this email directly or view it on GitHub.

@esmevane
Copy link

esmevane commented Aug 5, 2015

👍

3 similar comments
@mhutter
Copy link

mhutter commented Aug 8, 2015

👍

@jacobvosmaer
Copy link

👍

@mkarbowiak
Copy link

👍

@MrMarvin
Copy link

Are there any issues with this left open? I'd like to get this merged to get the warnings in my application disappear. Look good to me 👍

@vincentwoo
Copy link

👍

3 similar comments
@wolf99
Copy link

wolf99 commented Aug 31, 2015

👍

@n1moe1n
Copy link

n1moe1n commented Sep 1, 2015

👍

@wvolz
Copy link

wvolz commented Sep 2, 2015

👍

@jneen
Copy link
Member

jneen commented Sep 10, 2015

Closing for now, and seconding the intent to not depend on require_relative.

@jneen jneen closed this Sep 10, 2015
@jneen
Copy link
Member

jneen commented Sep 10, 2015

This should be fixed in v1.10.0.

@stanhu
Copy link
Contributor Author

stanhu commented Sep 10, 2015

Thanks and welcome back, @jneen! Glad to see the new release.

I already submitted an update to GitLab:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/1277

@jneen
Copy link
Member

jneen commented Sep 10, 2015

:]

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.