vaadin / maven-plugin

Vaadin Maven Plug-in
4 stars 18 forks source link

Close files so a generated widget set can be deleted on Windows (#20478) #77

Closed Artur- closed 7 years ago

Artur- commented 7 years ago

Fixes #74


This change is Reviewable

elmot commented 7 years ago

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


src/main/java/com/vaadin/integration/maven/UpdateWidgetsetMojo.java, line 121 at r1 (raw file):

                    OutputStream out = new FileOutputStream(appwsFile);
                    IOUtil.copy(template, out);
                    out.close();

Should be done in finally block


Comments from Reviewable

elmot commented 7 years ago

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.


src/main/java/com/vaadin/integration/maven/UpdateWidgetsetMojo.java, line 130 at r1 (raw file):

                FileInputStream fis = new FileInputStream(appwsFile);
                String generatedws = IOUtil.toString(fis);
                fis.close();

Must be done in finally block


Comments from Reviewable

Artur- commented 7 years ago

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.


src/main/java/com/vaadin/integration/maven/UpdateWidgetsetMojo.java, line 121 at r1 (raw file):

Previously, elmot (Ilia Motornyi) wrote… > Should be done in _finally_ block

why?


src/main/java/com/vaadin/integration/maven/UpdateWidgetsetMojo.java, line 130 at r1 (raw file):

Previously, elmot (Ilia Motornyi) wrote… > Must be done in _finally_ block

why?


Comments from Reviewable

elmot commented 7 years ago

Reviewed 1 of 1 files at r1. Review status: all files reviewed at latest revision, 2 unresolved discussions.


src/main/java/com/vaadin/integration/maven/UpdateWidgetsetMojo.java, line 121 at r1 (raw file):

Previously, Artur- (Artur) wrote… > why?

To be more or less sure it's really closed despite of any possible exceptions


src/main/java/com/vaadin/integration/maven/UpdateWidgetsetMojo.java, line 130 at r1 (raw file):

Previously, Artur- (Artur) wrote… > why?

To be more or less sure it's really closed despite of any possible exceptions


Comments from Reviewable

johannesh2 commented 7 years ago

Please merge this asap. Really annoying to have clean package always doing 47 seconds of widgetset compilation. I expected this to be fixed at least in 8 beta.

Peppe commented 7 years ago

Framework team, I also think this is very needed improvement that should be have a high priority. Takes me 1min 31s to run a project with all deps downloaded, on a powerful desktop PC. The first impressions are vital to users.

tsuoanttila commented 7 years ago

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions.


src/main/java/com/vaadin/integration/maven/UpdateWidgetsetMojo.java, line 121 at r1 (raw file):

Previously, elmot (Ilia Motornyi) wrote…
To be more or less sure it's really closed despite of any possible exceptions

I will make the requested changes to make this patch progress.


src/main/java/com/vaadin/integration/maven/UpdateWidgetsetMojo.java, line 130 at r1 (raw file):

Previously, elmot (Ilia Motornyi) wrote…
To be more or less sure it's really closed despite of any possible exceptions

I will make the requested changes to make this patch progress.


Comments from Reviewable

tsuoanttila commented 7 years ago

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions.


src/main/java/com/vaadin/integration/maven/UpdateWidgetsetMojo.java, line 121 at r1 (raw file):

Previously, tsuoanttila (Teemu Suo-Anttila) wrote…
I will make the requested changes to make this patch progress.

Reviewable messed up commits. never mind.


src/main/java/com/vaadin/integration/maven/UpdateWidgetsetMojo.java, line 130 at r1 (raw file):

Previously, tsuoanttila (Teemu Suo-Anttila) wrote…
I will make the requested changes to make this patch progress.

see above


Comments from Reviewable

tsuoanttila commented 7 years ago

src/main/java/com/vaadin/integration/maven/UpdateWidgetsetMojo.java, line 129 at r2 (raw file):

                // not have sufficient API to only generate the widgetset when it is needed
                try (FileInputStream fis = new FileInputStream(appwsFile)) {
                    String generatedws = IOUtil.toString(fis);

generatedws variable is used in the following if-block. Compilation error


Comments from Reviewable

Artur- commented 7 years ago

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.


src/main/java/com/vaadin/integration/maven/UpdateWidgetsetMojo.java, line 121 at r1 (raw file):

Previously, tsuoanttila (Teemu Suo-Anttila) wrote…
Reviewable messed up commits. never mind.

Done.


src/main/java/com/vaadin/integration/maven/UpdateWidgetsetMojo.java, line 130 at r1 (raw file):

Previously, tsuoanttila (Teemu Suo-Anttila) wrote…
see above

Done.


src/main/java/com/vaadin/integration/maven/UpdateWidgetsetMojo.java, line 129 at r2 (raw file):

Previously, tsuoanttila (Teemu Suo-Anttila) wrote…
generatedws variable is used in the following if-block. Compilation error

Done.


Comments from Reviewable

CLAassistant commented 7 years ago

CLA assistant check
All committers have signed the CLA.

elmot commented 7 years ago

Reviewed 2 of 3 files at r2, 1 of 1 files at r4. Review status: all files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

elmot commented 7 years ago
:lgtm:

Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

tsuoanttila commented 7 years ago

Reviewed 2 of 3 files at r2, 1 of 1 files at r4. Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable