Plugin variables #981

Fatme · 2015-09-29T07:11:36Z

ligaz · 2015-09-29T07:52:53Z

lib/definitions/plugins.d.ts

+}
+
+interface IPluginVariableData {
+	default?: string;


Should this be called defaultValue?

teobugslayer · 2015-09-29T15:06:16Z

lib/definitions/plugins.d.ts

@@ -9,7 +9,8 @@ interface IPluginsService {

 interface IPluginData extends INodeModuleData {
 	platformsData: IPluginPlatformsData;
-	pluginPlatformsFolderPath(platform: string): string; 
+	pluginVariables: IDictionary<IPluginVariableData>;


Can we document these members with jsdoc?

Mitko-Kerezov · 2015-10-07T11:01:05Z

lib/services/plugin-variables-service.ts

+	private static PLUGIN_VARIABLES_KEY = "variables";
+
+	constructor(private $errors: IErrors,
+		private $fs: IFileSystem,


Unused dependency?

Mitko-Kerezov · 2015-10-07T12:24:24Z

lib/services/plugin-variables-service.ts

+					let promptSchema = {
+						name: pluginVariableName,
+						type: "input",
+						message: `Enter value for ${pluginVariableName} variable: `


$prompter adds a white space at the end of every prompt so the white space at the end here seems redundant

As a side note, if the user presses sets a false-like value for a plugin variable we will fail in savePluginVariablesInProjectFile function up above and the process will end which is no great UX.
Instead we can use $prompter's validate property like so

validate: (value: string) => !!value ? true : 'Please enter a value!'

Mitko-Kerezov · 2015-10-07T12:59:46Z

lib/services/project-data-service.ts

@@ -11,7 +11,8 @@ export class ProjectDataService implements IProjectDataService {
 	constructor(private $fs: IFileSystem,
 		private $staticConfig: IStaticConfig,
 		private $errors: IErrors,
-		private $logger: ILogger) {
+		private $logger: ILogger,
+		private $injector: IInjector) {


Unnecessary dependency

Mitko-Kerezov · 2015-10-07T14:04:43Z

👍 After rebasing, getting a green build, merging pull request in common, updating SHA and squashing
😺

teobugslayer · 2015-10-07T14:37:33Z

lib/services/plugin-variables-service.ts

+			this.executeForAllPluginVariables(pluginData, (pluginVariableData: IPluginVariableData) =>
+				(() => {
+					let pluginVariableValue = this.getPluginVariableValue(pluginVariableData).wait();
+					if(!pluginVariableValue) {


This could be extracted into a dedicated function ensurePluginVarValue (the name is just a suggestion), the same code is used in other places as well.

teobugslayer · 2015-10-07T14:42:52Z

lib/services/plugin-variables-service.ts

+				value = value[pluginVariableName];
+			} else {
+				value = pluginVariableData.defaultValue;
+				if(!value && helpers.isInteractive()) {


If not interactive, we should give at least a warning.

We talked in person and agreed that the code is correct

teobugslayer · 2015-10-07T14:59:14Z

lib/tools/broccoli/builder.ts

+                let future = new Future<void>();
+
+                pipeline.on('end', (err: Error, data: any) => {
+                if(err) {


This should be indented once on the right

teobugslayer · 2015-10-07T15:23:46Z

lib/services/plugins-service.ts

+					if(this.$fs.exists(pluginConfigurationFilePath).wait()) {
+						let tnsModulesDestinationPath = path.join(platformData.appDestinationDirectoryPath, constants.APP_FOLDER_NAME, constants.TNS_MODULES_FOLDER_NAME);
+						let nodeModules = this.$broccoliBuilder.getChangedNodeModules(tnsModulesDestinationPath, platform).wait();
+						let plugins = _.map(_.keys(nodeModules), nodeModule => this.convertToPluginData(this.getNodeModuleData(pluginName).wait()));


This is only a suggestion and not a merge stopper:

_.(nodeModules) .map(nodeModule => this.getNodeModuleData(pluginName).wait()) .map(nodeModuleData => this.convertToPluginData(nodeModuleData)) .filter(data => data.isPlugin && this.$fs.exists(this.getPluginConfigurationFilePath(data, platformData)).wait()) .forEach((data, index) => { executeUninstallCommand().wait(); if(index === 0) { this.initializeConfigurationFileFromCache(platformData).wait(); } if(data.name !== pluginName) { this.merge(data, platformData).wait(); } }) .value();

I find this code to be more linear but the original is correct and I do not insist on changing it.

ns-bot · 2015-10-07T15:41:14Z

Test FAILed.

Plugin variables

ns-bot · 2015-10-07T16:05:26Z

Test FAILed.

Fatme added the feature label Sep 29, 2015

ligaz reviewed Sep 29, 2015
View reviewed changes

lib/definitions/plugins.d.ts

}

interface IPluginVariableData {

default?: string;

Copy link

ligaz Sep 29, 2015

Choose a reason for hiding this comment

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

Should this be called defaultValue?

Fatme force-pushed the fatme/plugin-vars branch 2 times, most recently from fd040ea to c9a1ec0 Compare September 29, 2015 11:30

teobugslayer reviewed Sep 29, 2015
View reviewed changes

teobugslayer added this to the 1.5.0 milestone Sep 30, 2015

Fatme force-pushed the fatme/plugin-vars branch 4 times, most recently from 3d73686 to 6c0ac6e Compare October 7, 2015 09:56

teobugslayer modified the milestones: 1.4.1, 1.5.0 Oct 7, 2015

Fatme force-pushed the fatme/plugin-vars branch from 6c0ac6e to 69a55ef Compare October 7, 2015 10:56

Mitko-Kerezov reviewed Oct 7, 2015
View reviewed changes

Fatme force-pushed the fatme/plugin-vars branch from 69a55ef to bc9dc86 Compare October 7, 2015 11:05

Mitko-Kerezov reviewed Oct 7, 2015
View reviewed changes

Fatme force-pushed the fatme/plugin-vars branch 2 times, most recently from cc325eb to 8e2d9b0 Compare October 7, 2015 12:52

Mitko-Kerezov reviewed Oct 7, 2015
View reviewed changes

Fatme force-pushed the fatme/plugin-vars branch 2 times, most recently from 0d0326a to 3bc89ae Compare October 7, 2015 14:20

Fatme force-pushed the fatme/plugin-vars branch from 3bc89ae to 9111e59 Compare October 7, 2015 14:34

teobugslayer reviewed Oct 7, 2015
View reviewed changes

Fatme force-pushed the fatme/plugin-vars branch from 9111e59 to 94cffd6 Compare October 7, 2015 14:41

teobugslayer reviewed Oct 7, 2015
View reviewed changes

Fatme force-pushed the fatme/plugin-vars branch from 94cffd6 to e9b26b3 Compare October 7, 2015 14:55

teobugslayer reviewed Oct 7, 2015
View reviewed changes

Fatme force-pushed the fatme/plugin-vars branch 2 times, most recently from 4049342 to 36d3d8e Compare October 7, 2015 15:14

teobugslayer reviewed Oct 7, 2015
View reviewed changes

Fatme force-pushed the fatme/plugin-vars branch from 36d3d8e to 8f2ffa0 Compare October 7, 2015 15:39

Plugin variables support

47716c3

Fatme force-pushed the fatme/plugin-vars branch from 8f2ffa0 to 47716c3 Compare October 7, 2015 15:42

Fatme pushed a commit that referenced this pull request Oct 7, 2015

Merge pull request #981 from NativeScript/fatme/plugin-vars

8d65522

Plugin variables

Fatme merged commit 8d65522 into master Oct 7, 2015

Fatme deleted the fatme/plugin-vars branch October 7, 2015 16:00

teobugslayer modified the milestones: 1.4.2, 1.4.1 Oct 14, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugin variables #981

Plugin variables #981

Fatme commented Sep 29, 2015

ligaz Sep 29, 2015

teobugslayer Sep 29, 2015

Mitko-Kerezov Oct 7, 2015

Mitko-Kerezov Oct 7, 2015

Mitko-Kerezov Oct 7, 2015

Mitko-Kerezov Oct 7, 2015

Mitko-Kerezov commented Oct 7, 2015

teobugslayer Oct 7, 2015

teobugslayer Oct 7, 2015

Fatme Oct 7, 2015

teobugslayer Oct 7, 2015

teobugslayer Oct 7, 2015

teobugslayer Oct 7, 2015

ns-bot commented Oct 7, 2015

ns-bot commented Oct 7, 2015

Plugin variables #981

Plugin variables #981

Conversation

Fatme commented Sep 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mitko-Kerezov commented Oct 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ns-bot commented Oct 7, 2015

ns-bot commented Oct 7, 2015