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

allowed nested transforms #2

Merged
merged 4 commits into from
Sep 17, 2015
Merged

allowed nested transforms #2

merged 4 commits into from
Sep 17, 2015

Conversation

djulien
Copy link
Contributor

@djulien djulien commented Sep 12, 2015

Added a stack so transforms can be nested for a given extension.

NOTE: had to comment out require("collections/shim") at line 43 of nested qt/q/q.js module; seems like there might be a file missing there somewhere

The original code only allowed one transform for each file extension.
This change allows multiple, nested transforms for each file extension
by keeping a stack instead of just a previous value.
added dummy defs to avoid run-time errors if Module is not present
@bahmutov
Copy link
Owner

Can you write a unit test / demo example?

@djulien
Copy link
Contributor Author

djulien commented Sep 13, 2015

Yes, but I am not up to speed on the testing conventions so this could take a little time.
Should it just be a simple test case that is really more like a demo, or should it try to test many possible combinations? (I assume that this feature would not be used much since nobody else has needed it - in that case I hesitate to add a lot of extra baggage in there)

Also, should I do anything about the missing shim file, or just leave that as is?

@bahmutov
Copy link
Owner

I already fixed the collections/shim - this is known problem with Q.v2, just pull this repo into your fork again to get the change

Take a look at the npm run demo-use command - it just runs a simple example. In your case create another example that applies two transforms to a required file and then console.assert that the loaded object has been transformed by both, ok?

@djulien
Copy link
Contributor Author

djulien commented Sep 14, 2015

Okay, I will pull it again.
I will try to add a test for 2 transforms on the same extension using the demo use-case example. thanks

Added a test case for nested hooks.  see test/use-case-nested.js
@djulien
Copy link
Contributor Author

djulien commented Sep 17, 2015

I added a test case for nested hooks and it passes.
Do I need to make another pull request or is this one still active?

bahmutov added a commit that referenced this pull request Sep 17, 2015
allowed nested transforms
@bahmutov bahmutov merged commit b915569 into bahmutov:master Sep 17, 2015
@bahmutov
Copy link
Owner

Published as 0.2.0, thanks for the feature!

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.

2 participants