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 obsolete types from public API #410

Closed
adamralph opened this issue Dec 1, 2014 · 10 comments
Closed

Remove obsolete types from public API #410

adamralph opened this issue Dec 1, 2014 · 10 comments

Comments

@adamralph
Copy link
Contributor

Following on from #408, there may be other parts of the API which we feel are redundant. It would be good to remove anything we find in 2.0.

@blairconrad
Copy link
Member

I like this idea. Since we've already got a "look at the whole world" issue (#374) in flight, does it make sense to combine them?

@adamralph
Copy link
Contributor Author

I think they're slightly different (or at least that was my understanding). This issue is for removing stuff from the public API which is simply not used any more by 99% of users.

#374 is about hiding the 'SDK' part of FakeIteasy, i.e. the part not meant to be used for writing tests, either by making stuff internal or putting it in an Sdk namespace.

@blairconrad
Copy link
Member

True #374 was framed as not negatively affecting clients, but that was back when we weren't already breaking the world for a 2.0 release. The mandate could've been (and could become) "hide or remove anything the outside world shouldn't see". I don't mind keeping the issues separate, but just thought it would be double scanning, and two places to check to see if we should make notes or whatever.
Carry on as you see fit. :-)

@blairconrad
Copy link
Member

If nothing else, if the work is to be carried out separately, it's probably a good idea to complete the review of the public API before #374. This issue is probably easier, more urgent, and may just a little bit make #374 easier, if some things are remove. I don't think the advantages flow in the other direction.

@adamralph
Copy link
Contributor Author

I agree that it's probably best to review the entire API just once. Perhaps we should indeed do that over in #374 and then this issue can be a spin off to remove stuff which we just think is obsolete.

@adamralph adamralph changed the title Review API for obsolete members Remove obsolete members from public API Dec 1, 2014
@blairconrad
Copy link
Member

@adamralph, I figure my best next move is to start working on removing some of the superfluous types and methods. I figure you're the most likely reviewer, so what's easiest for you? I'm content with an issue per removal or so, linked here or in the review task, or if you prefer we can just accumulate changes in one branch that we can review and merge in one go.
Or something smarter that you'll think of.

Thanks.

@adamralph
Copy link
Contributor Author

Great 👍

When you say 'issue per removal' do you mean 'PR per removal'? I don't think we need more than this one issue. I would just send a PR whenever you feel like it. E.g. when you've removed one member, or a closely related set of members. That PR can stay open whilst you continue working, and you can push more commits to it during that time. If at any time I merge to master, you can just rebase and open a new PR. Sound good?

@blairconrad
Copy link
Member

I would've gone as granular as issue per removal if that's what you wanted, but it did seem to be a little much.
PR per removal sounds fine, and "ever-growing PR as a backlog" like you describe works for me. I'm not that picky!

Thanks heaps!

blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Dec 19, 2014
blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Dec 19, 2014
blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Dec 19, 2014
blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Dec 19, 2014
blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Dec 19, 2014
blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Dec 19, 2014
blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Dec 23, 2014
blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Dec 23, 2014
blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Dec 23, 2014
blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Dec 23, 2014
blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Dec 23, 2014
blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Dec 23, 2014
blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Jan 4, 2015
blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Jan 4, 2015
blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Jan 4, 2015
blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Jan 4, 2015
blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Jan 4, 2015
blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Jan 4, 2015
@blairconrad
Copy link
Member

At this point #421 now contains everything that I think can and should be removed.

In the interest of having smaller issues and PRs, I'll start a new issue for internalizing some types and members.

@blairconrad
Copy link
Member

This issue has been fixed in release 2.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants