Conversation
ctucker3
commented
Apr 12, 2021
Contributor
Author
ctucker3
left a comment
There was a problem hiding this comment.
A bunch of comments on renaming to try and make class purpose more clear from the name. There are a few different groups of classes:
- Micronaut classes (controller, services, dao) -> Like we usually do.
- Import Configurations (generates import configuration and provides the class to map to)
- Mapping Classes (structs and managers to handle the mapping from file to import configs)
- Import Classes (classes that work with import configurations to POST the data)
src/main/java/org/breedinginsight/brapps/importer/model/BrAPIImportConfigManager.java
Outdated
Show resolved
Hide resolved
src/main/java/org/breedinginsight/brapps/importer/model/base/BrAPIPreview.java
Outdated
Show resolved
Hide resolved
src/main/java/org/breedinginsight/brapps/importer/model/base/BrAPIPreviewResponse.java
Outdated
Show resolved
Hide resolved
src/main/java/org/breedinginsight/brapps/importer/model/base/BrAPIPreviewStatistics.java
Outdated
Show resolved
Hide resolved
src/main/java/org/breedinginsight/brapps/importer/model/mapping/BrAPIMappingValue.java
Outdated
Show resolved
Hide resolved
src/main/java/org/breedinginsight/brapps/importer/services/BrAPIFileImportService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/breedinginsight/brapps/importer/services/BrAPIFileService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/breedinginsight/brapps/importer/services/BrAPIQueryService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/breedinginsight/brapps/importer/services/BrAPIQueryService.java
Outdated
Show resolved
Hide resolved
ctucker3
commented
Apr 12, 2021
src/main/java/org/breedinginsight/brapps/importer/model/imports/PedigreeImportService.java
Outdated
Show resolved
Hide resolved
nickpalladino
requested changes
Apr 14, 2021
src/main/java/org/breedinginsight/brapps/importer/controllers/ImportController.java
Outdated
Show resolved
Hide resolved
src/main/java/org/breedinginsight/brapps/importer/model/base/ObservationUnit.java
Show resolved
Hide resolved
src/main/java/org/breedinginsight/brapps/importer/model/mapping/BrAPIMappingManager.java
Show resolved
Hide resolved
src/main/java/org/breedinginsight/brapps/importer/controllers/ImportController.java
Outdated
Show resolved
Hide resolved
src/main/java/org/breedinginsight/brapps/importer/services/BrAPIFileImportService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/breedinginsight/brapps/importer/services/BrAPIQueryService.java
Outdated
Show resolved
Hide resolved
Member
nickpalladino
left a comment
There was a problem hiding this comment.
What's the plan for tests and/or docs? I know I asked in an earlier iteration and it was being pushed.
Contributor
Author
I think a separate card for tests and a separate card for docs. I think docs should come first. I wanted to get everything approved before spending time on tests/docs since things might undergo big changes before approval. |
added 20 commits
April 16, 2021 12:05
added 10 commits
April 16, 2021 12:05
a025322 to
3646863
Compare
timparsons
reviewed
Apr 16, 2021
src/main/java/org/breedinginsight/brapps/importer/controllers/ImportController.java
Outdated
Show resolved
Hide resolved
src/main/java/org/breedinginsight/brapps/importer/model/imports/PedigreeImportService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/breedinginsight/brapps/importer/services/FileImportService.java
Outdated
Show resolved
Hide resolved
54dd621 to
f463700
Compare
nickpalladino
approved these changes
May 3, 2021
timparsons
approved these changes
May 3, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This will work with the brapi server, but needs to create a trial and study object before it will work with breedbase. There are some breedbase endpoints that need to be fixed before this will work with breedbase, see this card:
https://breedinginsight.atlassian.net/secure/RapidBoard.jspa?rapidView=1&projectKey=BI&modal=detail&selectedIssue=BI-782
This is a large PR, so it might not be possible to look through all of the code. Maybe look through it and get an understanding of what the flow is and if there is anything glaringly obvious?
There is a list of TODOs for improvements that I identified as I was working on it. You can see these in the
PedigreeImportService.javafile.The class names could be cleaned up a little better to be more distinct to better represent the stages in the importer. I was planning on doing this after I got the pedigree importer fully working.
TODOS - Cards to create: