Clean up unused members and functions #745
Conversation
Codecov Report
@@ Coverage Diff @@
## master #745 +/- ##
===========================================
+ Coverage 47.26% 47.46% +0.2%
+ Complexity 1468 1464 -4
===========================================
Files 178 177 -1
Lines 9703 9651 -52
Branches 673 672 -1
===========================================
- Hits 4586 4581 -5
+ Misses 4826 4779 -47
Partials 291 291
Continue to review full report at Codecov.
|
|
Thanks, @acj, and welcome to the codebase! There is a lot of room for improvements and your help is very much appreciated. I also wanted to make you aware of #736, which we will be merging probably today. Unfortunately, it will cause merge conflicts with this PR, but we can work them out together after that ;) You mention that you're starting to familiarize with the codebase. That's great! If you are starting to think about potential changes, it would be great to hear about them! :D You can hop into the Slack #briefcase-code channel or the forums. I'm currently on GMT+2. |
That's great to hear! Congrats. It shouldn't be too painful to rebase my changes once that PR lands.
Nothing specific yet, but I'd like to help improve Briefcase's performance (parsing, networking, etc) once I get my bearings.
Will do. I'm on GMT-4. |
acj
force-pushed
the
clean-up-unused-things
branch
from
e6f33c9
to
2b06b1e
Compare
|
@ggalmazor This is ready for another look when you have time. I rebased onto master after #736 was merged. Also, not sure how/where to report this: I tried to join the ODK Slack and got a |
|
Hey, @acj! Thanks for updating your PR. I've had a quick look and there are a couple of things we should discuss to see how you feel about them first. I know that IntelliJ is telling you that all those refactors are safe and that all those things are unused but:
I know that this can be frustrating and even annoying, but we do it to produce a sturdier and more approachable design in a safe way. It would be exciting to discuss these things over at Slack, once you're able to join. It would also be super valuable to hear your take on the "newer" parts to know if the design makes it harder or easier for someone to make changes. @yanokwa @lognaturel, do you know why @acj could be getting that particular error when joining Slack? OTOH, there are changes that I think it's safe to merge at this moment. How would you like me to proceed? |
|
Hey, thanks for this context and explanation. That approach makes sense to me, and it explains some of the patterns that I was seeing -- e.g. older code that seems unused, and newer code that sometimes had comments explaining that it would be used in the near future. This has been a useful exercise in learning the code, so I don't mind the extra iterations. If you wouldn't mind listing the changes that are safe to merge, and/or describing where the boundaries between the old and new implementations are, then I'll rework the branch so that it only includes the safe parts. |
There was a problem hiding this comment.
Alright, there you go :)
I tagged with new/revisit and old/legacy the ones I think we should avoid changing.
I've added some context and some questions. The questions are not directed at anyone in special (well, probably at me). They're there just to think about the context around that particular change.
| @@ -65,18 +65,6 @@ public SubmissionMetaData(XmlElement root) { | |||
| return submissionDate; | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Good catch! This is done with Iso8601Helpers
| @@ -160,16 +160,15 @@ public static BriefcaseFormDefinition resolveAgainstBriefcaseDefn(File tmpFormFi | |||
| // newDefn is considered identical to what we have locally... | |||
| result = DifferenceResult.XFORMS_IDENTICAL; | |||
| } else { | |||
| result = JavaRosaParserWrapper.compareXml(newDefn, existingXml, existingTitle, true); | |||
There was a problem hiding this comment.
old/legacy
This class should be eventually replaced by FormDefinition
| @@ -45,10 +45,6 @@ public void markAsCancelled(PullEvent.Cancel event) { | |||
| log.info("cancel requested: " + event.cause); | |||
| } | |||
|
|
|||
| public void reset() { | |||
There was a problem hiding this comment.
old/legacy
This class should be eventually removed by the use of the JobsRunner class
| @@ -17,9 +17,5 @@ | |||
| package org.opendatakit.briefcase.model; | |||
|
|
|||
| public class UpdatedBriefcaseFormDefinitionEvent { | |||
| public final BriefcaseFormDefinition definition; | |||
There was a problem hiding this comment.
old/legacy
This class is used by BriefcaseFormDefinition, which will be replaced by FromDefinition and doesn't publish this event.
|
|
||
| public class JobsRunnerTest { | ||
| private static final Logger log = LoggerFactory.getLogger(JobsRunnerTest.class); |
| @@ -70,22 +70,6 @@ public static String buildAggregateSubmissionDownloadXml(String instanceId, int | |||
| ""; | |||
| } | |||
|
|
|||
| public static String buildAggregateSubmissionDownloadXml(String instanceId, List<AggregateAttachment> attachments) { | |||
There was a problem hiding this comment.
new/revisit
Why aren't we using this? Maybe missing a test somewhere?
| @@ -67,23 +67,19 @@ | |||
| } | |||
| } | |||
|
|
|||
| private ExportConfiguration currentConf; | |||
There was a problem hiding this comment.
new/revisit
All the *PageObject classes have to be thoroughly reviewed.
UI component tests should be revisited to see how well they're helping us prevent bugs and see how we can add more tooling to have more of them.
| @@ -49,7 +49,7 @@ protected void onSetUp() { | |||
| @Test | |||
| @Ignore | |||
| public void export_dir_button_opens_a_file_dialog() { | |||
| component = ConfigurationPanelPageObject.setUpDefaultPanel(robot(), empty().build(), true, true); | |||
There was a problem hiding this comment.
I lost track of changes :D.
Adjust as needed depending on whether the setUpDefaultPanel has to lose the arg or not.
| @@ -70,17 +70,4 @@ void set(TriStateBoolean value) { | |||
| component.set(value); | |||
| }); | |||
| } | |||
|
|
|||
| private JRadioButton getRadioButton(TriStateBoolean value) { | |||
There was a problem hiding this comment.
new/revisit
Why aren't we using this? Maybe missing a test somewhere?
acj
force-pushed
the
clean-up-unused-things
branch
from
2b06b1e
to
de61672
Compare
|
@ggalmazor Thanks for the careful review and suggestions. I'm not sure whether the last commit (removing CompletableFutureHelpers and DatePickerMatchers) is acceptable, but the rest feels pretty good now. |
|
Awesome! Thanks for updating the PR, @acj! It's OK to remove |
There was a problem hiding this comment.
Ah, fewer “lines spent”! A nice feeling.
https://www.softwarequotes.com/showquotes.aspx?id=545&name=Dijkstra,Edsger
|
Great! We'll merge this as soon as we release v1.16.1 with a couple of bug fixes. |
Makes incremental progress on #214. I'm trying to become familiar with Briefcase by doing this work, so please let me know if anything is misplaced or inappropriate. If this is too broad, I can break it into multiple PRs.
What has been done to verify that this works as intended?
I've run the tests and done a bit of manual testing.
Why is this the best possible solution? Were any other approaches considered?
Straightforward cleanup task.
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?
I've limited this PR to removing unused code, so risk should be minimal. However, it's possible that subtle bugs could have been introduced.
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.