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 PrototypeFactoryDeprecated #338

Merged
merged 1 commit into from
Jun 8, 2020
Merged

Remove PrototypeFactoryDeprecated #338

merged 1 commit into from
Jun 8, 2020

Conversation

dcbriccetti
Copy link
Contributor

@dcbriccetti dcbriccetti commented Jul 15, 2018

This class is used to get values from a map, but nothing is ever put into the map. It is called in only one place, in XFormParser.buildInstanceStructure, to get a value (always null, because of the always-empty map). It is called only if node.getAttributeValue(NAMESPACE_JAVAROSA, "modeltype") is not null.

Do we support this “modeltype” attribute? I don’t find it used anywhere in ODK. If we don’t, we can pull out a bit more code, and if we don’t know we can go ahead with the change in this PR.

What has been done to verify that this works as intended?

I have satisfied myself that the code I removed is dead code, and that the same path will continue to be executed.

Are there any risks to merging this code? If so, what are they?

No.

@codecov-io
Copy link

codecov-io commented Jul 15, 2018

Codecov Report

Merging #338 into master will increase coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #338      +/-   ##
============================================
+ Coverage      48.2%   48.25%   +0.04%     
+ Complexity     2862     2861       -1     
============================================
  Files           239      238       -1     
  Lines         13509    13493      -16     
  Branches       2616     2615       -1     
============================================
- Hits           6512     6511       -1     
+ Misses         6159     6145      -14     
+ Partials        838      837       -1
Impacted Files Coverage Δ Complexity Δ
src/org/javarosa/core/model/QuestionDef.java 78.03% <ø> (ø) 48 <0> (ø) ⬇️
src/org/javarosa/xform/parse/XFormParser.java 65.38% <0%> (+0.2%) 229 <0> (ø) ⬇️
...org/javarosa/core/model/condition/Triggerable.java 69.42% <0%> (+1.65%) 25% <0%> (ø) ⬇️

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 e121bd2...b96b11e. Read the comment docs.

@dcbriccetti dcbriccetti changed the title WIP: Remove PrototypeFactoryDeprecated Remove PrototypeFactoryDeprecated Aug 2, 2018
@dcbriccetti dcbriccetti requested a review from lognaturel August 2, 2018 21:45
@dcbriccetti
Copy link
Contributor Author

I don’t know if it was needed, but I just rebased this on master.

@ggalmazor
Copy link
Contributor

My only concern is that any other third party could be using the PrototypeFactoryDeprecated.addNewPrototype(String name, Class prototype), and this change would affect them.

I don't know if this should be something to worry about, taking into account that the class is called PrototypeFactoryDeprecated since April 2016 (feels like we gave plenty of time to react for anyone using)

This class is used to get values from a map, but nothing is ever put into the map. The one place it’s called to get a (null) value, in XFormParser.buildInstanceStructure, is not exercised by any current tests.
@codecov-commenter
Copy link

Codecov Report

Merging #338 into master will increase coverage by 0.05%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #338      +/-   ##
============================================
+ Coverage     54.60%   54.65%   +0.05%     
  Complexity     3222     3222              
============================================
  Files           243      242       -1     
  Lines         13330    13314      -16     
  Branches       2564     2563       -1     
============================================
- Hits           7279     7277       -2     
+ Misses         5245     5232      -13     
+ Partials        806      805       -1     
Impacted Files Coverage Δ Complexity Δ
...main/java/org/javarosa/core/model/QuestionDef.java 79.54% <ø> (ø) 50.00 <0.00> (ø)
...ain/java/org/javarosa/xform/parse/XFormParser.java 66.10% <0.00%> (+0.20%) 252.00 <0.00> (ø)
.../org/javarosa/core/reference/ReferenceManager.java 61.70% <0.00%> (+1.06%) 21.00% <0.00%> (+1.00%)

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 515e867...73dbae8. Read the comment docs.

@lognaturel
Copy link
Member

Merging now because we're about to do a major version bump. Thanks!

@lognaturel lognaturel merged commit eed7164 into getodk:master Jun 8, 2020
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