Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
    David Burg
    @daviburg
    And isn't that what you are attempting already with ReadWordChar?
    I did a superficial code review. Probably we need to resolve the questions of breaking or break-ish changes first.
    I'm a bit frazzled today due to many at work priorities, so my apologies for the lack of depth in the code review.
    microalps
    @microalps
    ReadWordChar - yes that was my fix to try to keep compatibility but not affect performance for 'normal' names
    microalps
    @microalps
    Thanks for the code review. I commit changes, now to wait for 437 to finish and rebase on that. No need to review my changes until I rebase.
    David Burg
    @daviburg
    Ok. So at a high level, there is no functional behavior change from your performance optimization PR thanks to the backward compatibility ReadWordChar, correct?
    microalps
    @microalps
    That's what I intended for with my code. There are no tests to verify that other than a singular test with upside down question mark not passing.
    microalps
    @microalps
    However there will be new cases that fail. This is because it collected valid chars but ignored invalid ones.
    For example, I'm not sure what old version did with $x but new version will throw an exception. Based on what I saw,it skips the $ and uses plain x as the vqriable lookup.
    microalps
    @microalps
    (http://dotliquidmarkup.org/try-online lets me put $user.name but not $user. name - liquidjs doesn't let the former but the latter is OK)
    microalps
    @microalps
    Ruby liquid does some wonky stuff
    $user.name - works as user.name
    $user .name - works as user.name
    $user. name - sees only user
    None of this behavior is correct IMHO
    David Burg
    @daviburg
    I agree that these behaviors should be corrected while the risk of regression is high. User can easily have a typo in their Liquid templates which 'just worked' before. Do you agree?
    (That the risk to regress some customers is high)
    microalps
    @microalps
    I agree there is risk. I don't think we should worry about that though. If you'd like we can call it v2.2 but to hold back performance because of potential typos , even with an opt-in flag, is silly. The error would be apparent - not like changing Capitalize filter which would silently change behavior.
    microalps
    @microalps
    @daviburg let me know if we can merge #437 seems ok with me
    David Burg
    @daviburg
    Cosmetic comments. Ok to merge without or without addressing them.
    @microalps can't deploy to a cloud compute hosting service like ours a breaking change. We can only host one compiled library in service memory at a time so breaking changes need to be opt-in.
    Sorry for been a bit selfish.
    microalps
    @microalps
    So be it. That's the second PR on ice. I guess I'll have to justify more effort with real performance difference in our companys usage of it. Happy to see @robin-parker joining the development effort. Anyone is free to cherry pick the other changes that aren't being challenged
    microalps
    @microalps
    @daviburg so don't upgrade to 2.2 branch. If you aren't exposimg the optin to clients the upgrade is sort of useless.
    David Burg
    @daviburg
    PR#437 merged.
    @microalps it is our intention to expose the optin to clients. My colleague Rama's team is looking into fitting that to their schedule, as well as picking up all the good work from you and the community lately.
    Robin Parker
    @robin-parker
    @microalps for PR #444 I'm only really interested in the string filters (sha1/sha256) as I need them in a logic app I'm working on. If the Json filter dependency is contentious then I'm happy to remove it.
    microalps
    @microalps
    So why do we need to merge into the DotLiquid standard filters? It can be a standalone dll with additional filters. These aren't standard to liquid, they are Shopify specific from what I understand. They aren't implemented in other liquid ports, are they? e.g. they aren't documented on https://shopify.github.io/liquid/ or on https://liquidjs.com/filters/overview.html
    Really, I'm just concerned about this one line:
    Template.RegisterFilter(typeof(ShopifyFilters));
    We shouldn't register non-standard filters by default.
    I can see users that needed md5 - just like you have in your example for gravatar images - and implemented their own and that would conflict with the new additions. (This doesn't concern me with standard filters because anything standard is a understandably a reserved keyword)
    microalps
    @microalps
    @daviburg I updated #441 to combine opt-in flag work and parsing changes for you. Since this does introduce syntax on parse portion (previously it was only on render) this is now a v2.2 branch. I didn't include sort filters because they were incomplete, but I would appreciate maybe @robin-parker helping on that.
    Robin Parker
    @robin-parker
    Seems contradictory to have a feature request for Shopify filters (#384) if we don't want to include Shopify filters in the library.
    I don't think the additional dll approach will work for me as I'm invoking DotLiquid using the OOTB action in azure logic apps.
    microalps
    @microalps
    I will let @daviburg chime in here as he created that feature request for Json. What are your thoughts about including Shopify named methods by default without a registration? It seems like a breaking change for everyone that might have opted to design their own filters. I know I have Json as a custom filter, but MD5 and other hash stuff is likely to clash in some setups.
    David Burg
    @daviburg
    I didn't realize although Shopify had a json filter they didn't make it part of the standard.
    oh wait, I did realize that in my issue description but did not anticipate the consequences.
    So it's not so much that we shouldn't include them, is that we need yet again to flag them for some opt-in to avoid name clash with others' custom filters.
    microalps
    @microalps
    Opt-in would be to just register JsonFilters - we don't need another mechanism
    David Burg
    @daviburg
    Which might be simply to not register the filter type by default
    Ah ah, you beat me to it.
    microalps
    @microalps
    But Robin's question is how this would work in Azure logic apps - I think that's your field
    David Burg
    @daviburg
    Robin, for OOTB action in Azure Logic Apps, we will need to add an option to include registration of well known filters shipping from DotLiquid Nuget.
    So I think you contribution to the open source is sufficient once your PR merges, and the Logic Apps engineers have a bit of work to do to expose the option.
    microalps
    @microalps
    Do they need to expose the option or always register it for convenience? If they can't load their own, then Azure Logic Apps engineers don't need to worry about conflicts and can safely register for everyone
    And if that's the case, maybe we should have a standard DotLiquid.Json as a standalone with references to System.Text.Json (and maybe a DotLiquid.Json.Net for Newtonsoft alternative)
    David Burg
    @daviburg
    User cannot load their own at this point, but Logic App is increasingly becoming open source (e.g. new Logic App Standard offering comes as a package to host in your own environment along side other code), so likely needs some future proofing there.
    Not a bad idea to have a standalone json filter assembly to isolate the references.
    David Burg
    @daviburg
    Lotsa code in your PR microalps. @robin-parker could you also take a pass at it? I might be missing stuff.
    microalps
    @microalps
    And lots of different little changes all in the PR heading. It's a merge of two separate PRs. I would like to add filter exception messages as part of the opt-in to resolve #413
    microalps
    @microalps
    Added #413 and resolved the PR comments @daviburg
    microalps
    @microalps
    And added #393. @daviburg I'm ready to merge this PR when you sign off
    microalps
    @microalps
    @david_burg_twitter @daviburg do you know when you will be able to review?
    David Burg
    @daviburg
    I'm sorry, I'm behind on work and home tasks...
    I see links to issues, trying to figure out which PRs behind them are ready