search

jsoverson / preprocess

Preprocess HTML, JavaScript, and other files with directives based off custom or ENV configuration
Other
366 stars 80 forks source link

Modified to allow setting variable #85

Closed DerekTBrown closed 2 years ago

DerekTBrown commented 9 years ago

Created an @set feature to allow setting environment variables from preprocess commands.

mhulse commented 9 years ago

Not having looked at the code, this sounds very useful! :+1:

Thanks for your contribution @DerekTBrown! :octocat:

DerekTBrown commented 9 years ago

@jsoverson I've never really worked with coveralls, so I am not sure why it is decreasing code coverage, probably because I added a test, and it is not expecting that output.

Besides that, this is ready to merge.

Frizi commented 9 years ago

You sure it works with different clauses? Mixing things, @set will not work inside @foreach as expected, as we don't have real parser and instead of going trough files from start to end, we have multiple regex passes. This could be potentially "fixed" by creating one massive regex for all rules.

I'm working with something like @exec set('variable', 'value') right now as a workaround, and it really behaves very unexpectedly.

DerekTBrown commented 9 years ago

@Frizi, that is an issue with the design of this package, not of the @set method. This package operates linearly down the file, and it sounds like you want the package to parse in a more dynamic way than that. That would be at the least, a separate pull request, or perhaps a new architecture from the beginning.

Frizi commented 9 years ago

that is an issue with the design of this package, not of the @set method

Indeed that's true. We just need a good documentation with big red sign "don't use it with anything". But then, @set alone isn't very useful. I think you might want to at least make it compatible with "echo", by putting both to single regex and doing switch in the logic.

This package operates linearly down the file

That's the exact opposite. It operates in passes, going through each file from top to the bottom for each directive regex separately.

That would be at the least, a separate pull request, or perhaps a new architecture from the beginning.

Yep, and there is an idea to rewrite it to use PEG.js or something similar. That will solve the problem in general, but for now we need to live with workarounds.

DerekTBrown commented 9 years ago

@Frizi - it sounds like your complaints have more to do with the architecture of this package than my addition of the @set method. If you would like to create a different package using PEG.js, that would be awesome, but for what I need, this does exactly the trick. In fact, you dont need a single regex. Here is a working example in which I use it with the @echo method:

https://github.com/uikit/uikit/pull/1410