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

Split diffs using file headers #87

Closed
wants to merge 3 commits into from
Closed

Conversation

piranna
Copy link
Contributor

@piranna piranna commented Oct 29, 2015

This follow better the structure of the "Unified Diff" format instead of depending on optional non-standard metadata like "Index:" or "diff" lines. This metadata is still parsed and used, though, but now it's possible to include other more easily. The main advantage of this is that now it's possible to use applyPatches() also with single anonimous diffs, that allow to use download-manager to download and patch all the source code of NodeOS without corner cases.

@kpdecker
Copy link
Owner

This is too much to review. As I mentioned on your past PR, you need to focus on individual changes. Broad scale renames that appear to be simply stylistic, etc mask the true changes. Please file a separate PR with just the functional changes.

I've noticed that you are committing directly to your fork's master, which can compound this problem. When doing pull requests you're better off creating a topic branch with the isolated changes and you can then merge those changes into your own master and use them directly from there while waiting for changes to land upstream. Ex: gotwarlost/istanbul#422 is on the source-map-errors branch but I link to the master branch which could have multiple different active PRs merged in without impacting one another.

@kpdecker kpdecker closed this Oct 29, 2015
@piranna
Copy link
Contributor Author

piranna commented Oct 29, 2015

I've noticed that you are committing directly to your fork's master,

I'm doing it this way because some changes goes on top of another, for example the ones of the rename of index to diff, because in fact we are parsing a diff :-) Now that I have it working on my machine, I'll try to cherry-pick and refactor it in several pull-requests so it will be easier to review (although the content of each one will be mostly the same of each independent commit...).

@kpdecker
Copy link
Owner

To be clear, I'm not really interested in stylistic refactorings so don't burn too much time there. "diff" vs. "index" is somewhat subjective and changing doesn't warrant the potential for errors being introduced in my eyes.

@piranna
Copy link
Contributor Author

piranna commented Oct 29, 2015

#88 should have the bare minimum changes to allow this feature, with some minor improvements on the regexp. I'll left the bigger changes for upcoming pull-requests, since this change is the one is the only really needed by download-manager.

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