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 import hooks to allow relative paths to files #72

Merged
merged 1 commit into from Sep 7, 2013

Conversation

hisenb3rg
Copy link
Contributor

There were 2 fixes done regarding relative paths:
a) local path of the parsed file is given as a path to the parser
b) sprockets dependencies are correctly resolved in #depends_on method

There were 2 fixes done regarding relative paths:
a) local path of the parsed file is given as a path to the parser
b) sprockets dependencies are correctly resolved in #depends_on method
import_paths = data.scan(IMPORT_SCANNER).flatten.compact.uniq
import_paths.each do |path|
for path in import_paths
Copy link
Owner

Choose a reason for hiding this comment

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

Is that a stylistic pref or necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a pref so not necessary, altough really like the way it reads.
Btw: been asked twice about using for loop in the last two days :)

Copy link
Owner

Choose a reason for hiding this comment

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

It is not idiomatic Ruby, I am a polyglot too, and I love reading that syntax in the languages it is idiomatic in, but not Ruby. Enumerable FTW in Ruby.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is true what you say but I'm still having hard time buying it that it is actually better. It is really a bit philosophical question... and I never found a decent or objective argumentation why it is better to use #each. If you have one please share. But I do see (tiny) readability improvement and terser syntax (less syntax noise). And readable and terse syntax is something why I like ruby :)

And disclaimer: I have no problem at all if you wish to change it to using #each and if you feel it makes it a better fit to the project (more consistent as well).

Copy link
Owner

Choose a reason for hiding this comment

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

  • It is better to be idiomatic.
  • Changing things like this could give pull requests friction. Another example would be spaces in empty lines cleanup which some people have git setup to do. Adding that style to a PR just add's a bit of noise.
  • The technical reasons for each are a bike shed. However, all of Enumerable is built around the idiomatic collection.dosomething syntax. There are no alternatives for for each_with_index, inject, map, all?, etc, etc. So the question then becomes, why make each different when all other Enumerable options will never look that way.

FWIW, I did change it back :)

Copy link
Owner

Choose a reason for hiding this comment

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

Also, you do not have lexical closure of the block as well. For is just a loop.

Copy link
Owner

Choose a reason for hiding this comment

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

Here is a technical reason for blocks and using Enumerable#each.

puts "For Loop:"
for x in [1,2,3]
  n = x + 1
  puts x
end

puts "For Loop (leaks):"
puts "x: #{x}"
puts "n: #{n}"

puts "Enumerable Block:"
[1,2,3].each do |a|
  b = a + 1
  puts a
end

puts "Enumerable Block (closure):"
puts "a: #{a rescue nil}"
puts "b: #{b rescue nil}"

This outputs.

For Loop:
1
2
3
For Loop (leaks):
x: 3
n: 4
Enumerable Block:
1
2
3
Enumerable Block (closure):
a: 
b: 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'm familiar with this one, but it is more displaying differences than improvements one of another.

Re: being idiomatic: True in general, but in this case? Everyone understands it on first sight, it is easy on the brain :)
Re: chanigng style in PR: Yeah agree on this one completely, shouldn't do that and usually I don't, but sometimes it is easy to get carried away :)
Re: technical reasons. Ok, I see the biggest point in missing nicer syntax for with index, altough you can achive it with for obj, idx in array.each_with_index. Other operation go beyond the simple iteration, so I wouldn't even want to use it in a for loop at all..

Copy link
Owner

Choose a reason for hiding this comment

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

@jurglic Read my comments on closures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did. Not sure about the weight of this one. I've never experienced any issue because you don't have a closure and variables leak. And you use if statements without a closure too, don't you?

metaskills added a commit that referenced this pull request Sep 7, 2013
Fix import hooks to allow relative paths to files. Fixes #72 and #64.
@metaskills metaskills merged commit fa9f297 into metaskills:master Sep 7, 2013
@@ -24,6 +24,9 @@ def evaluate(scope, locals, &block)

def config_to_less_parser_options(scope)
paths = config_paths(scope) + scope.environment.paths
local_path = scope.pathname.dirname
paths += [local_path] unless paths.include? local_path

Copy link
Owner

Choose a reason for hiding this comment

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

Cleaning all this up now. FWIW, I am in the minority on this one, but I firmly believe that spaces go between – methods never in them. If a method has a space, I consider it a smell that the Ruby is not clean enough to read in a cluster, should be broken down to smaller units or have comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, tnx for explanation.

@metaskills
Copy link
Owner

Just wanted to say thanks for the PR. Working on the 2.4.1 release now!

@hisenb3rg
Copy link
Contributor Author

Cool, tnx for merging!

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