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

Feature metadata refactor #1

Conversation

phated
Copy link

@phated phated commented Jan 28, 2016

I have put together a fairly large refactor that builds upon your latest PR. I wanted to submit it as a PR to your fork so you can take a look at the diff. If you diff this branch to the current master of vinyl-fs, it is actually really clean. Your changes were overall pretty great but I couldn't follow the callbacks being passed around everywhere. I had refactored stuff so the stat call was still in the written callback but then I figured out why you were passing the callbacks around (to handle the outstream.end case). Since streams handle all this stuff we are trying to do rather nicely, I just left them up their own devices and created a closeFd method that is used in writeBuffer and writeDir. I want to refactor the stat method, as it is just a cut-paste of your implementation into a module and add a ton more tests, but I figured I'd send the bulk of it to you now. Feedback is greatly appreciated.

@erikkemperman
Copy link
Owner

I bet this looks clean against master, most new code is in fresh modules now :-) But as these helpers are exclusively used by lib/writeContents/*, would they not better live in there as well?

A subjective matter, of course, but I found the original back-and-forth between index and writeXXX, with appropriate comments in place, slightly less confusing then having a third level of modules.

More importantly though this looks to me like it does the same thing, so if you're much happier with this structure, great.

Are the changes in test/dest.js and test/symlink.js related to this metadata work?

@phated
Copy link
Author

phated commented Jan 28, 2016

@erikkemperman
Module location: I was planning to reorganize the structure after I sent you this, I just didn't want to mess with the diff prior to sending it your way.
Test changes: no, they were just completely broken when i turned on some other eslint options (no-undef) and weren't testing what they were supposed to (so those features don't actually exist and are breaking changes).

@erikkemperman
Copy link
Owner

@phated All right, I'm going to have to call it a night -- one more thing I noticed, it is now no longer obvious that the symlink case is not handled yet. I had a TODO somewhere, I think?

And having read the flow in execution order again, for both branches, the original structure strikes me as slightly less fragmented lexicographically -- if that makes any sense -- but of course I spent more time looking at that structure earlier :-P

Maybe @piranna could weigh in?

@phated
Copy link
Author

phated commented Jan 28, 2016

I'll also add @contra

@phated
Copy link
Author

phated commented Jan 28, 2016

Ref gulpjs#151 for clean diff against master

@phated phated force-pushed the blaine-feature-metadata-refactor branch 2 times, most recently from dbae7f4 to 1f1790c Compare January 29, 2016 00:59
@erikkemperman
Copy link
Owner

@phated Just to double check, you made this PR to my branch for communication purposes only, right? Or should I merge it at some point?

@phated
Copy link
Author

phated commented Jan 29, 2016

No need to merge it. I'll be rebasing the branch while adding tests and stuff and then I can merge my PR to master. Just wanted to show the diff with yours.

@erikkemperman
Copy link
Owner

All right, thought so but wanted to be sure.

@phated phated force-pushed the blaine-feature-metadata-refactor branch 6 times, most recently from 3d06dbd to fa202ca Compare February 9, 2016 22:59
@phated phated force-pushed the blaine-feature-metadata-refactor branch 6 times, most recently from c56fae1 to fd33945 Compare February 25, 2016 23:34
@phated phated force-pushed the blaine-feature-metadata-refactor branch 2 times, most recently from 64d3056 to 550edd5 Compare February 26, 2016 00:07
@phated phated force-pushed the blaine-feature-metadata-refactor branch from 8b1b39b to 0cd8f22 Compare February 26, 2016 00:52
@erikkemperman
Copy link
Owner

Metadata stuff finally resolved, closing this!

@phated phated deleted the blaine-feature-metadata-refactor branch April 1, 2016 23:59
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