macdude357 commented 4 years ago

Reduce scope of members for issue #214

Closes #214

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

Code compiles and tests pass

Why is this the best possible solution? Were any other approaches considered?

Used IntelliJ's code inspection to find members that could be changed to a more restrictive scope.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

None

Does this change require updates to documentation? If so, please file an issue at https://github.com/opendatakit/docs/issues/new and include the link below.

No

macdude357 commented 4 years ago

Sorry about that. I fixed my compile issues but branch master had an existing compile error that I don't know how to fix since it requires a new arg and I'm not sure what to put there:

briefcase/model/form/FormMetadataCommandsTest.java:82: error: method orElseThrow in class Optional cannot be applied to given types; FormMetadata actualFormMetadata = formMetadataPort.fetch(key).orElseThrow();

ggalmazor commented 4 years ago

You're right, @macdude357, sorry about that. It looks like I slipped some errors into master without noticing. I'm working on a v2.0 branch that sets the source language level to Java 11 and those errors you're getting are because the master branch has to be compiled at a language level of Java 8. I'll work on a solution asap so that you can rebase your branch and we can process this PR.

ggalmazor commented 4 years ago

Hi @macdude357! You should rebase master into your branch to solve the issues with mismatching java language level calls.

codecov-io commented 4 years ago

Codecov Report

Merging #825 into master will decrease coverage by 0.02%. The diff coverage is 47.05%.

@@             Coverage Diff              @@
##             master     #825      +/-   ##
============================================
- Coverage     48.68%   48.65%   -0.03%     
- Complexity     1643     1645       +2     
============================================
  Files           192      192              
  Lines         10314    10324      +10     
  Branches        743      743              
============================================
+ Hits           5021     5023       +2     
- Misses         4935     4943       +8     
  Partials        358      358

Impacted Files	Coverage Δ	Complexity Δ
src/org/opendatakit/briefcase/export/Model.java	`88.57% <ø> (ø)`	`55 <0> (ø)`	:arrow_down:
...it/aggregate/parser/BaseFormParserForJavaRosa.java	`25.98% <ø> (+0.07%)`	`18 <0> (ø)`	:arrow_down:
...ndatakit/briefcase/model/BriefcasePreferences.java	`41.55% <ø> (ø)`	`14 <0> (ø)`	:arrow_down:
...sed/transfer/sourcetarget/CentralServerDialog.java	`0% <ø> (ø)`	`0 <0> (ø)`	:arrow_down:
...ase/model/UpdatedBriefcaseFormDefinitionEvent.java	`75% <0%> (-25%)`	`1 <0> (ø)`
src/org/opendatakit/briefcase/pull/PullEvent.java	`40% <0%> (-10%)`	`1 <0> (ø)`
...opendatakit/briefcase/reused/http/CommonsHttp.java	`57.57% <0%> (-0.89%)`	`11 <0> (ø)`
...opendatakit/briefcase/model/TerminationFuture.java	`35.71% <0%> (ø)`	`2 <0> (ø)`	:arrow_down:
.../org/opendatakit/briefcase/model/form/FormKey.java	`85.71% <100%> (ø)`	`15 <0> (ø)`	:arrow_down:
...takit/briefcase/model/BriefcaseFormDefinition.java	`33.14% <100%> (+0.37%)`	`16 <1> (+1)`	:arrow_up:
... and 4 more

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 a2941e0...d5edb11. Read the comment docs.

macdude357 commented 4 years ago

I'll update the PR to get the code coverage numbers back up.

ggalmazor commented 4 years ago

I'll update the PR to get the code coverage numbers back up.

I wouldn't worry about it.

ggalmazor commented 4 years ago

I'll tag this one to be released in v1.8 because this PR can't be tested without a full regression test, which we only do in minor releases (going from v1.17 to v1.18)

ggalmazor commented 4 years ago

So, we decided to ship this in v1.17.2.

@kkrawczyk123, when testing this one, please, try to go through the normal use cases to ensure this PR doesn't break basic functionality. One platform (Windows) and One server (Aggregate on Tomcat) would be enough.

kkrawczyk123 commented 4 years ago

Testes with success! None regression has been identified on Windows. (according to the comment above)

@opendatakit-bot unlabel "needs testing" @opendatakit-bot label "behavior verified"

getodk-bot commented 4 years ago

ERROR: Label "behavior identified" does not exist and was thus not added to this pull request.

kkrawczyk123 commented 4 years ago

@opendatakit-bot label "behavior verified"

getodk / briefcase

Reduce scope of members #825

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

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Does this change require updates to documentation? If so, please file an issue at https://github.com/opendatakit/docs/issues/new and include the link below.

Codecov Report