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 the Throwable/Error feature #498

Merged
merged 2 commits into from
Nov 11, 2020

Conversation

cortinico
Copy link
Member

📄 Context

This is the outcome of the community poll we had in #321

📝 Changes

  • Removed all the Throwable/Error related code/asset
  • Removed all the @Deprecated method for such feature
  • Updated the layout to don't use a ViewPager/TabView anymore
  • Updated the DB Schema number to 5

📷 Screenshots

Screenshot_1604923593

🚫 Breaking

Yes, as we're removing a lot of methods as planned (this will go in 4.x)

🛠️ How to test

Mostly removing code so not much to test.

⏱️ Next steps

Should we extract all the removed code to a separate repo? (i.e. having a version of Chucker that works only for Throwable?)

@cortinico cortinico added this to the 4.0.0 milestone Nov 9, 2020
MiSikora
MiSikora previously approved these changes Nov 9, 2020
Copy link
Contributor

@MiSikora MiSikora left a comment

Choose a reason for hiding this comment

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

Nice!

I think we can unpin #321 after this gets merged.

Should we extract all the removed code to a separate repo? (i.e. having a version of Chucker that works only for Throwable?)

Community feedback was that it would be useful so we might do that at some point. But I wouldn't rush it before 4.x. Unless you just want to keep it in a repo for now so the code does not get lost.

@MiSikora MiSikora dismissed their stale review November 9, 2020 17:24

I'm dismissing my approval because I just noticed that README.md requires update as well. :)

@vbuberen
Copy link
Collaborator

vbuberen commented Nov 9, 2020

Looks good for me.

I think that we should switch to just Activity for transactions list in future PRs instead of having FragmentedView and some Fragment.

Talking about README - I returned to my suggestion of moving documentation to its own website/page and will present updated version for a feedback pretty soon.

@olivierperez
Copy link
Member

olivierperez commented Nov 10, 2020

LGTM
About the idea of moving the removed part in another repo I'm not sure it would be good. Only 10 out of 34 voters voted to keep it inside the same repo, but they didn't vote to have it in another repo.
BTW there are other nice libraries to do it, let them handle this responsibility 🥇

About the README, I like to have information of How to use the library in the root README. And not to have to search for it.

@cortinico
Copy link
Member Author

I'm dismissing my approval because I just noticed that README.md requires update as well. :)

Updated

I think that we should switch to just Activity for transactions list in future PRs instead of having FragmentedView and some Fragment.

Agree on doing this in a subsequent PR 👍

Community feedback was that it would be useful so we might do that at some point. But I wouldn't rush it before 4.x. Unless you just want to keep it in a repo for now so the code does not get lost.

Well the code won't "never" get lost. We can still restore the repo just before this PR and dump it in a separate module/repository. Let's wait for after the 4.x release and see if the community is requesting it in any form.

BTW there are other nice libraries to do it, let them handle this responsibility 🥇

+1 🚀

Talking about README - I returned to my suggestion of moving documentation to its own website/page and will present updated version for a feedback pretty soon.
About the README, I like to have information of How to use the library in the root README. And not to have to search for it.

On this point, I agree on having the README as the entry point. However we fail to keep it up to date + it's getting longer and longer. I think we reached a point where having a proper project website would be beneficial (easier to search for information and easier to reference pages in Issue responses, etc.)

@cortinico cortinico force-pushed the nc/321/cleanup-throwable-feature branch from 0fa3184 to 7c4cda4 Compare November 10, 2020 18:35
@cortinico cortinico merged commit f2d35fd into develop Nov 11, 2020
@cortinico cortinico deleted the nc/321/cleanup-throwable-feature branch November 11, 2020 17:41
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.

4 participants