Skip to content
This repository was archived by the owner on Oct 24, 2025. It is now read-only.

Conversation

@asottile
Copy link
Member

@asottile asottile commented Dec 9, 2015

No description provided.

@asottile
Copy link
Member Author

asottile commented Dec 9, 2015

@amcgregor apologies about the back-and-forth about iterators. After flipping back and forth between the two I ended up sticking with the for-loop since it was way simpler (and with tuples it's not actually any worse performance-wise / memory-wise)

@amcgregor
Copy link
Contributor

@asottile Hmm. The use case I was imaging was:

@Cache  # Pretend this is heavily cached.
@cast  # typecast using Py3 annotations
def custom_importer(url: URL):
    if not url.startswith('asset://'): return

    for style in Asset.objects.get(path=url.path).scalar('styles'):
        yield URL(url, fragment=style.reference).render(), style.content

style.content may be large, but the above would stream the individual style fragments out of the DB as they're added to the C side of the equation. Basically, a concrete list or tuple of all results would require interning all of those string values at once, Python-side. (And yeah, my caches and RPC systems can cache and/or defer generators.)

@asottile
Copy link
Member Author

asottile commented Dec 9, 2015

Yeah that makes sense. They still need to be concretely serialized before we pass them to libsass anyway so it's kinda not worth it so much. I think if we find it being a problem we can revisit it but without library support it's still going to be an issue (and at some eventual point they all need to be serialized to build the whole css document :))

@amcgregor
Copy link
Contributor

Certainly! I was just hoping to avoid excessive string duplication. (I use Pypy, so I don't have an expectation that the memory will be freed immediately upon the last reference decrement for an object; cleanup is deferred to "some point in the future", so early tends to be best. ;)

@asottile
Copy link
Member Author

asottile commented Dec 9, 2015

@amcgregor can you try this out and see if works for your usecases?

@amcgregor
Copy link
Contributor

LLVM Clang spits out a lot of warnings from the libsass/ast source:

libsass/src/ast.hpp:2082:18: warning: expression with side effects will be evaluated despite being used as an operand to 'typeid'
      [-Wpotentially-evaluated-expression]
      if (typeid(*(*this)[0]) == typeid(Type_Selector))

As well as some weirder ones:

warning: unknown warning option '-Werror=unused-command-line-argument-hard-error-in-future'; did you mean
      '-Werror=unused-command-line-argument'? [-Wunknown-warning-option]

And finally, during final linking:

ld: warning: object file (build/temp.macosx-10.6-intel-3.4/libsass/src/parser.o) was built for newer OSX version (10.7) than being linked (10.6)

None of these should particularly impact usage. After installation I can confirm that my generator use is correctly packed down into a tuple for delivery to the C side. Looks good! 💃

asottile added a commit that referenced this pull request Dec 13, 2015
@asottile asottile merged commit 13eb3f8 into sass:python Dec 13, 2015
@asottile asottile deleted the importer_callbacks branch December 13, 2015 19:30
@asottile
Copy link
Member Author

Good enough for me :) I'll release a version later today

@asottile
Copy link
Member Author

Thanks again for all your work on this :D

@amcgregor
Copy link
Contributor

@asottile It never hurts to help, though I only really got the ball rolling on it. :P Still looking forward to that version bump, though!

@asottile
Copy link
Member Author

Oh derp, I will definitely do it today. I got super busy last time heh.

@asottile
Copy link
Member Author

This is available in 0.10.0 \o/

@dahlia
Copy link
Member

dahlia commented Dec 16, 2015

👍
On Wed, Dec 16, 2015 at 3:50 AM Anthony Sottile [email protected]
wrote:

This is available in 0.10.0 \o/


Reply to this email directly or view it on GitHub
#110 (comment)
.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants