WIP: feat: remove defaults function #1313

dpschen · 2022-01-05T21:11:12Z

dpschen commented

2022-01-05 21:11:12 +00:00

No description provided.

dpschen changed title from ~~feat: remove defaults function~~ to WIP: feat: remove defaults function

2022-01-05 21:11:28 +00:00

frederick commented

2022-01-05 21:16:03 +00:00

Hi dpschen!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://1313-featureremove-defaults-function-from-models--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi dpschen! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://1313-featureremove-defaults-function-from-models--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.

konrad reviewed 2022-01-06 10:35:54 +00:00

src/models/emailUpdate.js Outdated

						
				@ -8,2 +4,2 @@

						}

					}

					newEmail = ''

					passwort = ''

konrad commented

2022-01-06 10:35:54 +00:00

Wow. I think that parameter isn't even used like that - should be password https://try.vikunja.io/api/v1/docs#tag/user/paths/~1user~1settings~1email/post

Wow. I think that parameter isn't even used like that - should be `password` https://try.vikunja.io/api/v1/docs#tag/user/paths/~1user~1settings~1email/post

dpschen commented

2022-02-13 22:06:48 +00:00

Is fixed by #1540

Is fixed by https://kolaente.dev/vikunja/frontend/pulls/1540

dpschen marked this conversation as resolved

dpschen commented

2022-01-06 14:36:17 +00:00

Well that failed more than I expected :D

dpschen force-pushed feature/remove-defaults-function-from-models from 253017d0dd to 200851259a

2022-01-06 14:38:12 +00:00

Compare

dpschen commented

2022-02-09 16:50:04 +00:00

@konrad I realized that what I want to achieve with this branch seems easier possible by annotating them with JSDOC:

class ExampleC {
  constructor(data) {
    // property types can be simply annotated, if they're set elsewhere
    /** @type {number} */
    this.size;
 
  }
}

What do you think?

@konrad I realized that what I want to achieve with this branch seems easier possible by [annotating them with JSDOC](https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html#classes-1): ```js class ExampleC { constructor(data) { // property types can be simply annotated, if they're set elsewhere /** @type {number} */ this.size; } } ``` What do you think?

konrad commented

2022-02-10 22:22:58 +00:00

If that works well, it could be a good way to solve it.

The long-term goal should be moving everything over to typescript though, not sure if using annotations would mean twice the effort?

If that works well, it could be a good way to solve it. The long-term goal should be moving everything over to typescript though, not sure if using annotations would mean twice the effort?

dpschen commented

2022-02-13 20:45:31 +00:00

This seems to work.
I tried it in #1536
The cool think is that it's not as strict as ts for the input values (when you create the model). This makes it easier to transition, because else you would need to format a lot of date before creating the class.
In the longterm we should find a solution similar to that.
Even though verbose this really makes things easier.

This seems to work. I tried it in https://kolaente.dev/vikunja/frontend/pulls/1536 The cool think is that it's not as strict as ts for the input values (when you create the model). This makes it easier to transition, because else you would need to format a lot of date before creating the class. In the longterm we should find a solution similar to that. Even though verbose this really makes things easier.

dpschen referenced this pull request

2022-02-13 22:13:33 +00:00

fix: property spelling #1540

dpschen commented

2022-02-14 10:10:47 +00:00

Closing this.
My plan is now:

incrementally add JSDOCs annotations
convert to ts where possible.
don't introduce new defaults functions for new models

This seems to be the easiest way forward.

Closing this. My plan is now: - incrementally add JSDOCs annotations - convert to ts where possible. - don't introduce new defaults functions for new models This seems to be the easiest way forward.

👍 1

dpschen closed this pull request

2022-02-14 10:10:47 +00:00

dpschen commented

2022-02-14 15:12:32 +00:00