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

Infer Sass syntax from item identifier extension, closes #1397 #1407

Merged
merged 1 commit into from Feb 16, 2019

Conversation

Projects
None yet
3 participants
@agross
Copy link
Contributor

agross commented Feb 11, 2019

Detailed description

Infer Sass or Scss syntax from identifier with fallback to Scss if the identifer has no extension.

This also fixes the specs that always used Scss syntax from CompiledContentStore, despite of the created TextualContents with the respective syntax.

Related issues

#1397, FYI @gpakosz

@gpakosz

This comment has been minimized.

Copy link
Member

gpakosz commented Feb 11, 2019

Hello @agross 👋

I apologize, I got side-tracked and didn't look at #1397. Doing it now

:scss,
import_all,
]
end

This comment has been minimized.

@gpakosz

gpakosz Feb 11, 2019

Member

I started changing this file last week but didn't go as far as you did with updating tests.
I wrote something similar:

      content, syntax = if items.size == 1
                          [items.first.compiled_content, items.first.ext.downcase.to_sym || :sass]
                        else
                          [items.map { |item| %(@import "#{item.identifier}";) }.join("\n"), :scss]
                        end

      options[:syntax] = syntax

Which imho produces a smaller diff than yours.

I chose to make it content, syntax because that's somewhat what Sass does, see find_relative().
I chose to make :sass the default syntax because that's also what Sass does.

This comment has been minimized.

@agross

agross Feb 11, 2019

Author Contributor

My primary goal is to write readable code, diff size doesn't matter much to me. Git stores snapshots anyways. 😉

The fallback is :scss because that's what's the original code was hard-coding.

I needed to change the specs because they were failing now that Sass compiles the inferred syntax, but the content served by the cache was Scss.

I'm open to exchanging syntax and content if that is preferred.

This comment has been minimized.

@gpakosz

gpakosz Feb 12, 2019

Member

Readable is a subjective matter. When I talk about minimizing the diff, I talk about the cognitive load of inspecting the diff, not Git's way of storing things. I let @ddfreyne decide.

Having content, syntax is symetric with what Sass does and what #find_relative() does.

As far as the current behavior of assuming :scss, it's a bug and I vaguely recall my first implementation before review inspected item's extension to decide the syntax. Maybe we could #raise() telling we don't know which syntax to pick?

This comment has been minimized.

@ddfreyne

ddfreyne Feb 14, 2019

Member

I don’t have much of an opinion here — if the tests pass and Rubocop is happy with it, then I’m happy.

This comment has been minimized.

@agross

agross Feb 15, 2019

Author Contributor

@gpakosz Made the changes as requested. I also added some new specs round importing partials.

This comment has been minimized.

@gpakosz

gpakosz Feb 15, 2019

Member

@agross Thanks for the update. Well my very own opinion is that I wouldn't have traded 5 lines of code in context for 3 additional private functions (see http://number-none.com/blow/john_carmack_on_inlined_code.html). Understanding what's going on imho now requires chasing tiny functions doing little meaningful work.

So yeah I personally would have written it differently. But if both Rubocop and @ddfreyne are happy then I'm happy too!

@agross agross force-pushed the agross:import-sass-by-id branch from 1ce36b2 to 57b8bc5 Feb 15, 2019

@ddfreyne ddfreyne merged commit e9972c1 into nanoc:master Feb 16, 2019

3 checks passed

codecov/patch Coverage not affected when comparing 6126026...57b8bc5
Details
codecov/project 98.33% (+<.01%) compared to 6126026
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@agross agross deleted the agross:import-sass-by-id branch Feb 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.