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

fix: android bitmap skip dynamic and handle exceptions #337

Merged
merged 4 commits into from Jun 14, 2019

Conversation

dpvreony
Copy link
Member

@dpvreony dpvreony commented Jun 8, 2019

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Mitigates type load exceptions as described in #330

What is the current behavior? (You can also link to an open issue here)
Can throw exceptions and not handled as graceful as they could be.

What is the new behavior (if this is a feature change)?
skip dynamic assemblies
catch exceptions, log them and return the types we did manage to load.

What might this PR break?
If dynamic assemblies are used for generating drawable resources, these will no longer load.
behaviour change that it logs out reflection type load exceptions but will continue if it can.

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Other information:

Needs a sense check
Also I've queried whether GetExportedTypes should be used instead?

@dpvreony dpvreony requested a review from a team June 8, 2019 06:37
@glennawatson
Copy link
Contributor

Thanks for doing this, was going to look into it this weekend but saves me the hassle.

@glennawatson
Copy link
Contributor

The build failure is weird, it seems to be borking on the temporary version string, which was previously always accepted. Wonder if they updated the Azure DevOps versions or something that has broken it. I will investigate.

@glennawatson
Copy link
Contributor

glennawatson commented Jun 8, 2019

The build issue is due to you having the rare case of a all numeric git commit id for the first 10 characters.

@glennawatson
Copy link
Contributor

If you commit something again it'll likely go away.

@dpvreony
Copy link
Member Author

dpvreony commented Jun 8, 2019

will push another commit when I get back

@glennawatson
Copy link
Contributor

glennawatson commented Jun 8, 2019

Correction, it's the rare case where you have a sequence starting with 0 and all number sequence.

@codecov
Copy link

codecov bot commented Jun 10, 2019

Codecov Report

Merging #337 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #337   +/-   ##
=======================================
  Coverage   70.68%   70.68%           
=======================================
  Files          65       65           
  Lines        3544     3544           
  Branches      316      316           
=======================================
  Hits         2505     2505           
  Misses        997      997           
  Partials       42       42

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 913f5a3...f1e4915. Read the comment docs.

shouldnt be needed if the gettypesfromassembly behaves on exceptions
@dpvreony
Copy link
Member Author

removed the dynamic filter piece as the exception handling block should cater for it

@glennawatson glennawatson merged commit a7aa168 into master Jun 14, 2019
@delete-merged-branch delete-merged-branch bot deleted the incompletedynamictypes branch June 14, 2019 05:24
@glennawatson glennawatson changed the title WIP: android bitmap skip dynamic and handle exceptions fix: android bitmap skip dynamic and handle exceptions Jun 14, 2019
dpvreony added a commit that referenced this pull request Jul 15, 2019
@lock lock bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants