Skip to content
This repository has been archived by the owner on Dec 5, 2019. It is now read-only.

Add constants to packages and cleanup code #32

Merged
merged 1 commit into from
Feb 16, 2018
Merged

Add constants to packages and cleanup code #32

merged 1 commit into from
Feb 16, 2018

Conversation

vchepeli
Copy link
Contributor

@vchepeli vchepeli commented Feb 15, 2018 •
edited
Loading

Describe what this PR does and why we need it:
We need to improve out QoC, add additional checks not null and throw additional runtime exceptions

Changes proposed in this pull request

  • Add Constants to packages
  • Change legacy code
  • Improve JSon output
  • Use threadpool executor for all CLIRunner tasks

@vchepeli vchepeli force-pushed the t_plugin_cleanup branch 3 times, most recently from 81a4f35 to 8fd0a50 Compare February 15, 2018 11:50
sedroche
sedroche reviewed Feb 15, 2018
View reviewed changes
src/main/java/org/aerogear/plugin/intellij/mobile/api/CLIRunnerImpl.java Outdated
public CLIRunnerImpl() {
this.notificationsService = new MobileNotificationsService();
this.executorService = Executors.newFixedThreadPool(EXECUTOR_NUM_THREADS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to create this per instance or for each call to executeAsync? I am unsure of how the Executor classes work

src/main/java/org/aerogear/plugin/intellij/mobile/services/sdkconfig/SDKConfigManager.java
Objects.requireNonNull(AeroGearMobileConfiguration.getInstance(project)).setConfigPath(path);
});
} else {
Objects.requireNonNull(AeroGearMobileConfiguration.getInstance(project)).setConfigPath(path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the file is not created why do we want to store the setting?

Copy link
Contributor Author

@vchepeli vchepeli Feb 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File you want to create already exists and you need only update your configPath to be same as path. But we need to test if this works

src/main/java/org/aerogear/plugin/intellij/mobile/ui/sdkconfig/NewFileForm.java Outdated
@@ -3,7 +3,7 @@
import javax.swing.JTextField;
import com.intellij.openapi.ui.TextFieldWithBrowseButton;

public class NewFileForm extends javax.swing.JPanel {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any classes that are auto-generated UI classes I would like to leave alone.

src/main/java/org/aerogear/plugin/intellij/mobile/ui/sdkconfig/ServiceDeployedNotification.java Outdated
String sdkConfigPath = AeroGearMobileConfiguration.getInstance(this.project).getConfigPath();
String content = "";
String sdkConfigPath = Objects.requireNonNull(AeroGearMobileConfiguration.getInstance(this.project)).getConfigPath();
StringBuilder content = new StringBuilder(); // TODO ADD template files if notification is redundant
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this TODO an issue? There is one created around template file already. Does that cover it?
#19

src/main/java/org/aerogear/plugin/intellij/mobile/ui/sdkconfig/ViewSDKConfigPanel.java Outdated

public ViewSDKConfigPanel() {
initComponents();
}

private void initComponents() {

scrollPane = new javax.swing.JScrollPane();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any classes that are auto-generated UI classes I would like to leave alone.
If it is could you put a comment in like the other auto-generated files?

src/main/java/org/aerogear/plugin/intellij/mobile/ui/servicecatalog/ServicePanel.java Outdated
@@ -14,7 +16,7 @@
* To edit this file, open the ServicePanel.form file in the NetBeans GUI Builder.
* To view the generated source code, choose Source within the Netbeans GUI Builder.
*/
public class ServicePanel extends javax.swing.JPanel {
class ServicePanel extends javax.swing.JPanel {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any classes that are auto-generated UI classes I would like to leave alone.

...g/aerogear/plugin/intellij/mobile/ui/servicecatalog/identity/IdentityConfigurationPanel.java Outdated
import com.intellij.ui.Gray;
import com.intellij.ui.JBColor;

import javax.swing.*;

/**
* This class is generated by and can be edited using the Netbeans GUI Builder.
* To edit this file, open the ServicePanel.form file in the NetBeans GUI Builder.
* To view the generated source code, choose Source within the Netbeans GUI Builder.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

@vchepeli vchepeli force-pushed the t_plugin_cleanup branch 6 times, most recently from a42f9e6 to 66115fa Compare February 15, 2018 14:48
dimitraz
dimitraz approved these changes Feb 15, 2018
View reviewed changes
Copy link
Contributor

@dimitraz dimitraz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@vchepeli vchepeli force-pushed the t_plugin_cleanup branch 4 times, most recently from a92dd42 to fc43c8b Compare February 16, 2018 12:30
Add constants to packages and cleanup code
6b1d076 
Fix icons when sources are processed
Process should have execution on timeout
witmicko
witmicko approved these changes Feb 16, 2018
View reviewed changes
Copy link
Contributor

@witmicko witmicko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good,
I am liking all QoL improvements.

@sedroche can you please verify that the features you had worked on are still working as intended.
If they are we could merge it.

@sedroche
Copy link
Contributor

All good from my point @witmicko @vchepeli

@vchepeli vchepeli merged commit 481f5f0 into master Feb 16, 2018
@vchepeli vchepeli deleted the t_plugin_cleanup branch February 16, 2018 16:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sedroche sedroche sedroche approved these changes

@JameelB JameelB JameelB approved these changes

@dimitraz dimitraz dimitraz approved these changes

@witmicko witmicko witmicko approved these changes

@maleck13 maleck13 Awaiting requested review from maleck13

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vchepeli @sedroche @witmicko @dimitraz @JameelB