new getAttributes methods #1357 #1392
Conversation
src/actions/GenericActions.js
Outdated
| @@ -38,6 +38,26 @@ export function getData( | |||
| ); | |||
| } | |||
|
|
|||
| /** | |||
| * | |||
| * @param {*} entity - entity should always be `documentView` | |||
There was a problem hiding this comment.
then why having it as parameter?
There was a problem hiding this comment.
just for the future development
There was a problem hiding this comment.
Agile development best practices(1) says that we shall not develop ahead and imagine things because they might never happen. It might be that there will be no future extension. And when and if it will be, we will address it at that time.
At the moment i find this misleading for new developers.
More I would rename the function from "getAttributes" to "getViewAttributes" because that's what precisely is doing.
I will drop that "entity" param because it;s pointless and i will also drop that viewId and rowId checking because if they are null/missing, you would build a wrong endpoint URL anyways.
Also, please note, I am not a javascript developer, so it might be wrong what I am saying.
I would like to hear other oppinions too.
@pablosichert @metas-ts @ottosichert guys, wdyt?
References:
- Clean Code: A Handbook of Agile Software Craftsmanship (Robert C. Martin)
src/actions/GenericActions.js
Outdated
| '/' + entity + | ||
| '/' + windowId + | ||
| (viewId ? '/' + viewId : '') + | ||
| (rowId ? '/' + rowId : '') + |
There was a problem hiding this comment.
imho the viewId and rowId shall always be present. if not, there is an error.
There was a problem hiding this comment.
yes, they will always be presented but left them to make it generic function
There was a problem hiding this comment.
@teosarca do you want me to update these comments?
|
found an issue: wrong endpoint is called when PATCHing: |
#1357