Handling of files for which no importer (or too many) is present does not fail with an exception by poldi2015 · Pull Request #258 · itsallcode/openfasttrace

poldi2015 · 2021-04-16T11:10:30Z

Implement files for ZIP File Importer
Improve ZIP File importer
Implement tests

…vailable

codecov · 2021-04-16T11:15:22Z

Codecov Report

Merging #258 (f4b6c6a) into develop (3f2af36) will decrease coverage by 0.03%.
The diff coverage is 86.66%.

❗ Current head f4b6c6a differs from pull request most recent head f116915. Consider uploading reports for the commit f116915 to get more accurate results

@@              Coverage Diff              @@
##             develop     #258      +/-   ##
=============================================
- Coverage      87.29%   87.25%   -0.04%     
  Complexity      1034     1034              
=============================================
  Files            120      120              
  Lines           3187     3193       +6     
  Branches         249      250       +1     
=============================================
+ Hits            2782     2786       +4     
  Misses           335      335              
- Partials          70       72       +2

Impacted Files	Coverage Δ	Complexity Δ
...de/openfasttrace/importer/zip/ZipFileImporter.java	`44.44% <ø> (ø)`	`3.00 <0.00> (ø)`
...fasttrace/core/importer/MultiFileImporterImpl.java	`91.48% <80.00%> (-3.86%)`	`19.00 <4.00> (ø)`
...fasttrace/core/importer/ImporterFactoryLoader.java	`94.11% <100.00%> (+0.78%)`	`9.00 <0.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 3f2af36...f116915. Read the comment docs.

…le to deal with any type of file. If no importer is present than nothing is imported (silently, with a LOG.info message in ImporterFactoryLoader). No special handling in ZipFileImporter necesssary anymore.

…into feature/257-only_import_support_files_from_zip_files

kaklakariada · 2021-04-18T15:17:06Z

Bitte noch bei Bedarf Requirements und Design aktualisieren :)

kaklakariada · 2021-05-02T09:09:47Z

Build for PR from fork failed because of missing SONAR_TOKEN. We still can merge this.
@redcatbear Please also review.

poldi2015 · 2021-05-22T20:51:13Z

Can we merge this pull request to develop or is something still open? Should @redcatbear also do a review?

kaklakariada · 2021-05-23T06:11:42Z

Yes, he should also review.

redcatbear · 2021-05-23T10:30:22Z

+
+

Unnecessary change.

redcatbear · 2021-05-23T10:31:40Z

     *             when no or more than one {@link ImporterFactory} is found.
     */
-    public ImporterFactory getImporterFactory(final InputFile file)
+    public Optional<ImporterFactory> getImporterFactory(final InputFile file)


Please split into check method and getter method instead of using Optional. Use getter only if check comes back green.

This would mean that the ImporterFactory needs to be searched twice: once by the check method to figure out that there is a fitting ImporterFactory and second by the get method. Th search calls ImporterFactory.supportsFile() on each factory when then figures out by examine the given file if it can load the file.

What is the problem with Optional. As Java has no Either construct, isn't Optional not the Java approach for such a construct?

redcatbear · 2021-05-23T10:33:02Z

+        LOG.fine(() -> ( importer.isPresent() ?
+                            "Created importer of type '" + importer.getClass().getSimpleName() :
+                            "No import" )


Suggested change

LOG.fine(() -> ( importer.isPresent() ?

"Created importer of type '" + importer.getClass().getSimpleName() :

"No import" )

LOG.fine(() -> (importer.isPresent() ?

"Created importer of type '" + importer.getClass().getSimpleName() :

"No import")

Please use auto-formatting. that should also make sure that all spaces are uniform.

redcatbear · 2021-05-24T07:02:46Z

Build failing.

kaklakariada · 2021-05-24T09:27:37Z

Build fails during sonar analysis with message Caused by: org.sonar.api.utils.MessageException: You're not authorized to run analysis. Please contact the project administrator.

Root cause is, that pr from forks don't have access to the sonar token. I fixed this issue on develop by skipping sonar in this case.

I will merge the pr as is. We can fix potential sonar findings later.

…t_files_from_zip_files Handling of files for which no importer (or too many) is present does not fail with an exception

Changed ZipFileImporter to only read files for which an importer is a…

ba37845

…vailable

Reworked handling of no matching importer: MultiFileImporter is no ab…

b41c6e9

…le to deal with any type of file. If no importer is present than nothing is imported (silently, with a LOG.info message in ImporterFactoryLoader). No special handling in ZipFileImporter necesssary anymore.

poldi2015 changed the title ~~WIP: Changed ZipFileImporter to only read files for which an importer is a…~~ Changed ZipFileImporter to only read files for which an importer is a… Apr 17, 2021

poldi2015 changed the title ~~Changed ZipFileImporter to only read files for which an importer is a…~~ Handling of files for which no importer (or too many) is present does not fail with an exception Apr 17, 2021

Merge branch 'develop' of https://github.com/itsallcode/openfasttrace …

f4b6c6a

…into feature/257-only_import_support_files_from_zip_files

kaklakariada approved these changes Apr 17, 2021

View reviewed changes

Comment thread core/src/main/java/org/itsallcode/openfasttrace/core/importer/MultiFileImporterImpl.java

Comment thread core/src/test/java/org/itsallcode/openfasttrace/core/importer/TestImporterFactoryLoader.java Outdated

Worked in review comments of kaklakariada

f116915

kaklakariada requested a review from redcatbear April 18, 2021 15:17

kaklakariada approved these changes May 2, 2021

View reviewed changes

redcatbear requested changes May 23, 2021

View reviewed changes

redcatbear approved these changes May 24, 2021

View reviewed changes

kaklakariada merged commit 1104526 into itsallcode:develop May 24, 2021

kaklakariada mentioned this pull request May 24, 2021

Prepare release 3.2.0 #272

Merged

poldi2015 pushed a commit that referenced this pull request Jul 11, 2021

Merge pull request #258 from poldi2015/feature/257-only_import_suppor…

b00dfb6

…t_files_from_zip_files Handling of files for which no importer (or too many) is present does not fail with an exception

poldi2015 pushed a commit that referenced this pull request Jul 11, 2021

Merge pull request #258 from poldi2015/feature/257-only_import_suppor…

4b24a94

…t_files_from_zip_files Handling of files for which no importer (or too many) is present does not fail with an exception

Conversation

poldi2015 commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

kaklakariada commented Apr 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaklakariada commented May 2, 2021

Uh oh!

poldi2015 commented May 22, 2021

Uh oh!

kaklakariada commented May 23, 2021

Uh oh!

redcatbear May 23, 2021

Choose a reason for hiding this comment

Uh oh!

poldi2015 May 23, 2021

Choose a reason for hiding this comment

Uh oh!

redcatbear May 23, 2021

Choose a reason for hiding this comment

Uh oh!

poldi2015 May 23, 2021

Choose a reason for hiding this comment

Uh oh!

redcatbear May 23, 2021

Choose a reason for hiding this comment

Uh oh!

poldi2015 May 23, 2021

Choose a reason for hiding this comment

Uh oh!

redcatbear commented May 24, 2021

Uh oh!

kaklakariada commented May 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

poldi2015 commented Apr 16, 2021 •

edited

Loading

codecov Bot commented Apr 16, 2021 •

edited

Loading

kaklakariada commented Apr 18, 2021 •

edited

Loading