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.

Sign up for GitHub

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

Jump to bottom

auto-patching operators: Fixes and updates #1097

Merged
merged 4 commits into from
Jan 11, 2016
Merged

auto-patching operators: Fixes and updates #1097

merged 4 commits into from
Jan 11, 2016

Conversation

luisgabriel
Copy link
Contributor

Closes to #1030.

@luisgabriel
Copy link
Contributor Author

Let me do some more tests before merging it. ;)

@kwonoj
Copy link
Member

kwonoj commented Dec 18, 2015

Suuuuure. Is this PR closes issue completely which includes injecting comments also?

@luisgabriel
Copy link
Contributor Author

@kwonoj Not yet. I'll add some commits injecting the comments.

@luisgabriel luisgabriel changed the title Fix patch generation script auto-patching operators: Fixes and updates Dec 21, 2015
@luisgabriel
Copy link
Contributor Author

@kwonoj Now it's complete. ;)

kwonoj
kwonoj reviewed Dec 22, 2015
View reviewed changes
tools/generate-operator-patches.ts
@@ -57,19 +58,26 @@ const AliasMethodOverrides = {
};

function generateNewOperatorFileContents (op:OperatorWrapper): OperatorWrapper {
var header = `/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can these variable can be const? I know it's script and not for actual codebases though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I'll change this one. I did not touch the others because they're not related to the patch.

@luisgabriel
Copy link
Contributor Author

@kwonoj Just updated the PR addressing you comments.

@kwonoj
Copy link
Member

kwonoj commented Dec 22, 2015

I think change's good, thinking which changes should be checked in first between #1077 and this. Still need some comment update from those PR regarding patch operators.

@luisgabriel
Copy link
Contributor Author

I think it doesn't make a difference which one gets merged first. If it is necessary to keep the change regarding the patch operators on #1077, it will require a separate PR to change the generator script.
What do you think?

@kwonoj
Copy link
Member

kwonoj commented Dec 23, 2015

I think it doesn't make a difference which one gets merged first.

: Yes I agree, just work order in my preferences :)

@luisgabriel
Copy link
Contributor Author

Given the size of #1077, maybe it would be better to merge it first as is. Then I can rebase and update this PR.

This was referenced Dec 23, 2015
Merged
Closed
@luisgabriel
Copy link
Contributor Author

I'll update this PR as soon as #1091 gets merged.

@kwonoj kwonoj added the blocked label Jan 11, 2016
@kwonoj
Copy link
Member

kwonoj commented Jan 11, 2016

@luisgabriel , PR #1091 's checked in to allow this PR can be updated. Thanks for take care of those. Let me mark as blocked for now to avoid possible confusions.

@luisgabriel
Copy link
Contributor Author

Thanks, @kwonoj! I'll rebase and update this PR tomorrow.

@luisgabriel
chore(tools): fix generate-operator-patches script
3f0f81d 
There is no `extended` directory anymore. Now, in order to check if an
operator is extended we check if it's declared in KitchenSink.
@luisgabriel
chore(tools): fix generate-operator-patches script
5e3ef04 
After ReactiveX#1010, the script that generates the operator patches was not
updated to include the typescript hack that was manually added to the
generated files in the codebase.
@luisgabriel
chore(tools): add comments to generated files
9ce8dd9
@luisgabriel
refactor(operator): add comments to generated operator files
5e43f14
@luisgabriel
Copy link
Contributor Author

Just updated this PR. Now it includes also pairwise, race and let that were recently checked in.

@kwonoj kwonoj merged commit 5e43f14 into ReactiveX:master Jan 11, 2016
@kwonoj
Copy link
Member

kwonoj commented Jan 11, 2016

Merged with 5e43f14, appreciate for effort @luisgabriel :)

@luisgabriel luisgabriel deleted the fix-patch-generation branch January 11, 2016 18:12
luisgabriel referenced this pull request Feb 3, 2016
@luisgabriel @kwonoj
chore(tools): remove script to generate operator patches
bb308f4 
This is not being used anymore.
@lock
Copy link

lock bot commented Jun 7, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
blocked
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@luisgabriel @kwonoj