fix(cloud-templates): cast default number and boolean numbers to feel #121
Conversation
marstamm
requested review from
barmac and
abdul99ahad
and removed request for
a team
| @@ -3,6 +3,7 @@ import { getBusinessObject } from 'bpmn-js/lib/util/ModelUtil'; | |||
| import { is, isAny } from 'bpmn-js/lib/util/ModelUtil'; | |||
|
|
|||
| import { v4 as uuid } from 'uuid'; | |||
| import { isSpecialFeelProperty, toFeelExpression } from './util/feelUtil'; | |||
There was a problem hiding this comment.
Minor nit-pick: Let's rename the utility to CamelCase, i.e. FeelUtil.
I acknowledge that we already have some lowerCaseYesThatIsMe files in the code-base, but let's not introduce too many more of them, as the standard is starting with a capital letter.
There was a problem hiding this comment.
Is that the convention we have? As I see it, the file name reflects the default export. Per convention, Classes and Components are written in PascalCase, hence a lot of our files are also PascalCase.
The utility files do not have a default export or exports a function. Since a function is usually camelCase (ignoring functional components), the file name will reflect this as well.
This is consistent with all existing utility files
There was a problem hiding this comment.
Cf. https://github.com/bpmn-io/bpmn-js-element-templates/blob/e8d563a5b4b5abcdaf88ffada30fd9c0c914c95e/src/cloud-element-templates/util/validate.js as an example.
Validator is a class, since it is in Validator.js. When making it a utility validate, the file name will reflect the contents and is named validate.js (lower case)
There was a problem hiding this comment.
I can only look towards our different utilities in the eco-system, and they all seem to be historically upper-case - https://github.com/bpmn-io/diagram-js/tree/develop/lib/util.
Only recently we started lower-casing things in side-projects, but also not consistently. Predominently, utilities are still upper case, unless imported via index.js directly - https://github.com/bpmn-io/bpmn-js-element-templates/tree/main/src/utils.
👍 on reflecting the name / type of the default export, where it makes sense. But your utility does not provide a default export, but is a collection of various things - https://github.com/bpmn-io/bpmn-js-element-templates/pull/121/files#diff-ed2c638650f4cb053378f288c21bb35939630f122e9d0eb79f4924d62984a084.
Another debate is if "single function per file", with a default context is an appropriate way to slice things. Sometimes (but only sometimes) it makes sense.
There was a problem hiding this comment.
I agree that we are not consistent in our naming schemes. In the context of this JSX project, I would argue that using camelCase for utilities helps with understanding the file structure better. Using Upercase primarily for Components helps with knowing what the contents of the file will be without having to open the file itself.
Predominently, utilities are still upper case
Only if they are closely related to a bpmn-js utility they are extending:
But your utility does not provide a default export, but is a collection of various things
While the utility file does not have a default export, it still only exports functions and should therefore follow the naming convention for functions, rather than Components
There was a problem hiding this comment.
No hard opinion, but if we want to keep naming consistent, I'd follow the unofficial core libraries rule of PascalCase everywhere (cf. bpmn-js and diagram-js).
There was a problem hiding this comment.
Adjusted to PascalCase
There was a problem hiding this comment.
In the end, stuff is usually auto-imported. For that reason, I'd discourage default exports.
related to camunda/camunda-modeler#4517
Proposed Changes
We already cast Number and Boolean values to feel when they are changed, but keep the default value. This leads to problems when the default value is a
trueinstead of"=true".This PR casts the default value to feel if the property requires it.
Checklist
To ensure you provided everything we need to look at your PR:
@bpmn-io/srtoolCloses {LINK_TO_ISSUE}orRelated to {LINK_TO_ISSUE}