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

Remove ESDoc related stuff #5462

Merged

Conversation

jakovljevic-mladen
Copy link
Member

Hi everyone. Here's a cleanup PR. I'm not completely sure that this should be done, so I've split code changes to three commits, where I hope that at least one or two of them might be merged, if not all.

  • I removed a script that was added with this commit 2ef4682. Based on Deps audit #4771, ESDoc is removed, so it makes sense to remove it now.
  • Also, since @owner is removed (51a860b) from most of the docs, I have removed almost all other @owner leftovers. There's only one place where @owner appears: Observable's create method.
  • Since ESDoc is not being used anymore, it makes sense not to have block comments that clearly stated that they were used to affect ESDoc.

Please let me know if these changes make sense. If some of them don't, I can remove them.

@cartant cartant requested a review from niklas-wortmann May 28, 2020 22:39
@niklas-wortmann
Copy link
Member

This is an amazing pull request. Thanks a lot for taking care of this. I need to check it out locally. I'll try to take care of this, this weekend and come back to you!

@jakovljevic-mladen
Copy link
Member Author

Thank you @niklas-wortmann. I hope you won't have problems when you try this.

@jakovljevic-mladen jakovljevic-mladen force-pushed the remove_esdoc_related_stuff branch from e8d2226 to b4c446f Compare June 18, 2020 06:44
@jakovljevic-mladen
Copy link
Member Author

Hey @niklas-wortmann, since this PR changes lots of files, a since lots of conflicts occur over time, please let me know once you start your review, so I can rebase and resolve conflicts at that time. Or, if you want to do it on your own, please do.

@jakovljevic-mladen jakovljevic-mladen force-pushed the remove_esdoc_related_stuff branch from b4c446f to 8d344f5 Compare October 3, 2020 21:12
@niklas-wortmann
Copy link
Member

pinging @cartant, @kolodny, @benlesh just to double check that this does not affect the generated d.ts files

@kolodny
Copy link
Member

kolodny commented Oct 9, 2020

🤷‍♂️ I wouldn't worry about it IMO, if folks start complaining then we can see what we can do. My gut tells me this is already pretty safe

Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for splitting the changes into separate commits, @jakovljevic-mladen This was very easy to review. ❤

@benlesh benlesh merged commit 8a0712e into ReactiveX:master Oct 13, 2020
@benlesh
Copy link
Member

benlesh commented Oct 13, 2020

Thank you, @jakovljevic-mladen!

@jakovljevic-mladen jakovljevic-mladen deleted the remove_esdoc_related_stuff branch October 14, 2020 06:29
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.

5 participants