You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by GitBox <gi...@apache.org> on 2021/08/05 15:10:09 UTC
[GitHub] [brooklyn-ui] ahgittin opened a new pull request #263: Parameter defaults
ahgittin opened a new pull request #263:
URL: https://github.com/apache/brooklyn-ui/pull/263
Broader fix for issue observed in #261
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-ui] jathanasiou commented on a change in pull request #263: Parameter defaults
Posted by GitBox <gi...@apache.org>.
jathanasiou commented on a change in pull request #263:
URL: https://github.com/apache/brooklyn-ui/pull/263#discussion_r683551336
##########
File path: ui-modules/blueprint-composer/app/components/spec-editor/spec-editor.template.html
##########
@@ -323,7 +323,7 @@ <h4>No matching parameters</h4>
<div ng-if="model.miscData.get('config').length === 0">
<h4>No configuration</h4>
<p class="buttons">
- <button class="btn btn-sm btn-success" ng-click="specEditor.addConfigKey(state.config.search)" ng-if="state.config.search">
+ <button class="btn btn-sm btn-success" ng-click="specEditor.addConfigKey(state.config.search)" ng-if="state.config.search && state.config.search!=''">
Review comment:
Don't believe this is needed as it adds complexity and empty `''` strings evaluate to false in `if` statements anyways. https://stackoverflow.com/questions/27380000/angularjs-ng-if-check-string-empty-value
##########
File path: ui-modules/blueprint-composer/app/components/util/model/entity.model.js
##########
@@ -685,7 +685,17 @@ function addConfigKeyDefinition(param, overwrite, skipUpdatesDuringBatch) {
let key = (param || {}).name;
if (!key) throw new Error("'name' field must be included when adding parameter; was", param);
- allConfig[key] = Object.assign(allConfig[key] || {}, param, overwrite ? null : allConfig[key]);
+ let paramMapped = Object.assign({}, param);
Review comment:
I feel confident the below would be a much cleaner version of this function with the same behaviour. I'd really suggest something close for maintenance purposes if it feels accurate logic-wise.
```javascript
function addConfigKeyDefinition(param, overwrite, skipUpdatesDuringBatch) {
const allConfig = this.miscDataOrDefault('configMap', {});
if (typeof param !== 'undefined') {
const parameterItem = (typeof param !== 'string')
? { ...param } // shallow copy as object
: {
"name": param,
"label": param,
"description": "",
"priority": 1,
"pinned": true,
"type": "java.lang.String",
"constraints": [],
};
const key = get(parameterItem, 'name'); // import { get } from 'lodash' at top, defaults to undef.
// this will also throw if original `param` is neither object nor string due to previous statement
if (!key) throw new Error("'name' field must be included when adding parameter; was", param);
if (typeof parameterItem.default !== 'undefined') {
/* Annoyingly, in parameters, we call the default value "default",
* but in config, we call them "defaultValue";
* when params are merged to config we need to rename
*/
parameterItem.defaultValue = parameterItem.default;
delete parameterItem.default;
}
Object.assign(allConfig[key], parameterItem);
}
if (!skipUpdatesDuringBatch) {
this.miscData.set('config', Object.values(allConfig));
}
this.touch();
return this;
}
```
##########
File path: ui-modules/blueprint-composer/app/components/spec-editor/spec-editor.template.html
##########
@@ -332,7 +332,7 @@ <h4>No configuration</h4>
<h4>No matching configuration</h4>
<p class="buttons">
<button class="btn btn-sm btn-default" ng-if="state.config.search.length > 0" ng-click="state.config.search = ''">Clear search</button>
- <button class="btn btn-sm btn-default" ng-click="specEditor.addConfigKey(state.config.search)" ng-if="!specEditor.getConfig(state.config.search)">
+ <button class="btn btn-sm btn-default" ng-click="specEditor.addConfigKey(state.config.search)" ng-if="state.config.search && state.config.search!='' && !specEditor.getConfig(state.config.search)">
Review comment:
Same as previous.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-ui] jathanasiou commented on a change in pull request #263: Parameter defaults
Posted by GitBox <gi...@apache.org>.
jathanasiou commented on a change in pull request #263:
URL: https://github.com/apache/brooklyn-ui/pull/263#discussion_r683602770
##########
File path: ui-modules/blueprint-composer/app/components/util/model/entity.model.js
##########
@@ -685,7 +685,17 @@ function addConfigKeyDefinition(param, overwrite, skipUpdatesDuringBatch) {
let key = (param || {}).name;
if (!key) throw new Error("'name' field must be included when adding parameter; was", param);
- allConfig[key] = Object.assign(allConfig[key] || {}, param, overwrite ? null : allConfig[key]);
+ let paramMapped = Object.assign({}, param);
Review comment:
Yeah fair, just though I'd share. For the record, `overwrite` was removed because it seemed to contribute no different logic path to the end result due to the syntax I went with that allowed a simpler extension.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-ui] ahgittin commented on a change in pull request #263: Parameter defaults
Posted by GitBox <gi...@apache.org>.
ahgittin commented on a change in pull request #263:
URL: https://github.com/apache/brooklyn-ui/pull/263#discussion_r683597921
##########
File path: ui-modules/blueprint-composer/app/components/util/model/entity.model.js
##########
@@ -685,7 +685,17 @@ function addConfigKeyDefinition(param, overwrite, skipUpdatesDuringBatch) {
let key = (param || {}).name;
if (!key) throw new Error("'name' field must be included when adding parameter; was", param);
- allConfig[key] = Object.assign(allConfig[key] || {}, param, overwrite ? null : allConfig[key]);
+ let paramMapped = Object.assign({}, param);
Review comment:
slightly clearer and more efficient apart from (1) you've lost the `overwrite` behaviour and (2) bringing in `get` obscures behaviour for those not familiar with `_.get` -- but the benefit is so slight it isn't worth the effort of testing it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-ui] jathanasiou commented on a change in pull request #263: Parameter defaults
Posted by GitBox <gi...@apache.org>.
jathanasiou commented on a change in pull request #263:
URL: https://github.com/apache/brooklyn-ui/pull/263#discussion_r683580595
##########
File path: ui-modules/blueprint-composer/app/components/util/model/entity.model.js
##########
@@ -685,7 +685,17 @@ function addConfigKeyDefinition(param, overwrite, skipUpdatesDuringBatch) {
let key = (param || {}).name;
if (!key) throw new Error("'name' field must be included when adding parameter; was", param);
- allConfig[key] = Object.assign(allConfig[key] || {}, param, overwrite ? null : allConfig[key]);
+ let paramMapped = Object.assign({}, param);
Review comment:
I feel confident the below would be a much cleaner version of this function with the same behaviour. I'd really suggest something closer to this for maintenance purposes if it feels accurate logic-wise.
```javascript
function addConfigKeyDefinition(param, overwrite, skipUpdatesDuringBatch) {
const allConfig = this.miscDataOrDefault('configMap', {});
if (typeof param !== 'undefined') {
const parameterItem = (typeof param !== 'string')
? { ...param } // shallow copy as object
: {
"name": param,
"label": param,
"description": "",
"priority": 1,
"pinned": true,
"type": "java.lang.String",
"constraints": [],
};
const key = get(parameterItem, 'name'); // import { get } from 'lodash' at top, defaults to undef.
// this will also throw if original `param` is neither object nor string due to previous statement
if (!key) throw new Error("'name' field must be included when adding parameter; was", param);
if (typeof parameterItem.default !== 'undefined') {
/* Annoyingly, in parameters, we call the default value "default",
* but in config, we call them "defaultValue";
* when params are merged to config we need to rename
*/
parameterItem.defaultValue = parameterItem.default;
delete parameterItem.default;
}
Object.assign(allConfig[key], parameterItem);
}
if (!skipUpdatesDuringBatch) {
this.miscData.set('config', Object.values(allConfig));
}
this.touch();
return this;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-ui] ahgittin commented on pull request #263: Parameter defaults
Posted by GitBox <gi...@apache.org>.
ahgittin commented on pull request #263:
URL: https://github.com/apache/brooklyn-ui/pull/263#issuecomment-894089390
thx for the review @jathanasiou -- merging
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-ui] ahgittin commented on a change in pull request #263: Parameter defaults
Posted by GitBox <gi...@apache.org>.
ahgittin commented on a change in pull request #263:
URL: https://github.com/apache/brooklyn-ui/pull/263#discussion_r683592451
##########
File path: ui-modules/blueprint-composer/app/components/spec-editor/spec-editor.template.html
##########
@@ -323,7 +323,7 @@ <h4>No matching parameters</h4>
<div ng-if="model.miscData.get('config').length === 0">
<h4>No configuration</h4>
<p class="buttons">
- <button class="btn btn-sm btn-success" ng-click="specEditor.addConfigKey(state.config.search)" ng-if="state.config.search">
+ <button class="btn btn-sm btn-success" ng-click="specEditor.addConfigKey(state.config.search)" ng-if="state.config.search && state.config.search!=''">
Review comment:
that's what i thought @jathanasiou but for some reason without this i was seeing an `Add ''` button and adding this seemed to fix it! not sure what is going on.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-ui] ahgittin commented on a change in pull request #263: Parameter defaults
Posted by GitBox <gi...@apache.org>.
ahgittin commented on a change in pull request #263:
URL: https://github.com/apache/brooklyn-ui/pull/263#discussion_r684040900
##########
File path: ui-modules/blueprint-composer/app/components/util/model/entity.model.js
##########
@@ -685,7 +685,17 @@ function addConfigKeyDefinition(param, overwrite, skipUpdatesDuringBatch) {
let key = (param || {}).name;
if (!key) throw new Error("'name' field must be included when adding parameter; was", param);
- allConfig[key] = Object.assign(allConfig[key] || {}, param, overwrite ? null : allConfig[key]);
+ let paramMapped = Object.assign({}, param);
Review comment:
if `overwrite` was false or set false because `param` was supplied as a string, then the difference is that the code should do `Object.assign(parameterItem, allConfig[key])` prior to the `Object.assign(allConfig[key], parameterItem)` -- the difference being if both set a value for the same key, which one should be kept. With this change any key set in the param object will always overwrite such a key in config.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-ui] asfgit closed pull request #263: Parameter defaults
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #263:
URL: https://github.com/apache/brooklyn-ui/pull/263
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [brooklyn-ui] jathanasiou commented on a change in pull request #263: Parameter defaults
Posted by GitBox <gi...@apache.org>.
jathanasiou commented on a change in pull request #263:
URL: https://github.com/apache/brooklyn-ui/pull/263#discussion_r683600477
##########
File path: ui-modules/blueprint-composer/app/components/spec-editor/spec-editor.template.html
##########
@@ -323,7 +323,7 @@ <h4>No matching parameters</h4>
<div ng-if="model.miscData.get('config').length === 0">
<h4>No configuration</h4>
<p class="buttons">
- <button class="btn btn-sm btn-success" ng-click="specEditor.addConfigKey(state.config.search)" ng-if="state.config.search">
+ <button class="btn btn-sm btn-success" ng-click="specEditor.addConfigKey(state.config.search)" ng-if="state.config.search && state.config.search!=''">
Review comment:
Sounds like an angular 1 bug. I saw some mentions online of how `ng-if` evaluates expressions so maybe we're victims of that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brooklyn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org