Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added --return-errors flag #122

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

wiegell
Copy link

@wiegell wiegell commented Dec 7, 2020

On second thought it should maybe export to entirely different function, so you could have both the regular type guard and this new one concurrently.
Thoughts?

documentation

fix

fix

fix
@wiegell
Copy link
Author

wiegell commented Dec 7, 2020

Hmm... actually there's an error in my code, which i suspect also exist in the --debug mode.
When chaining the evaluate() functions with && chrome will stop running the functions after hitting the first false. Therefore it should not be able to print more than one error to console.error() (haven't tested this though), or in my case not be able to push more than one error to the array.
Will try and fix, though it requires som rewriting...

@wiegell wiegell marked this pull request as draft December 8, 2020 15:26
@wiegell wiegell marked this pull request as ready for review December 9, 2020 23:27
@wiegell
Copy link
Author

wiegell commented Dec 9, 2020

Ok completely rewrote to output new function. Comments are welcome. Feels like it could use some cleanup.

@rhys-vdw
Copy link
Owner

rhys-vdw commented Dec 22, 2020

Hey @wiegell, thanks for your contribution! I would have preferred to discuss a change like this in an issue before you opened a PR.

tbh I had already considered such a feature, but not as a global flag. The current ts-auto-guard:type-guard is meant to leave room for other types of functions instead of type-guard. For example this could be ts-auto-guard:validator. I know the "export-all" functionality kind of clashes with this... It could be updated to export-all:validator,type-guard or something.

Either way I don't want to just have a flag for how the entire thing works because debug is meant for debugging only so it makes sense... A flag to switch your entire code base means that you can't choose between both!

So, this is my proposal:

/** 
 * @see {isFoo} ts-auto-guard:type-guard
 * @see {validateFoo} ts-auto-guard:validator
 **/
 interface Foo { /* ... */ }

@rhys-vdw
Copy link
Owner

Also if this is just intended for debugging then I'd be happy to update the debug functionality to just fire off a single error which includes the full information of mismatched fields...

Let me know what this is for.

@wiegell
Copy link
Author

wiegell commented Dec 22, 2020

Hey @wiegell, thanks for your contribution! I would have preferred to discuss a change like this in an issue before you opened a PR.

Yea i just thought i would have to do a minor tweak and all of a sudden i had written a lot of code. And i wanted to get going using it in my project. So i had already written the code and i thought i might as well make the PR instead of an issue :)

tbh I had already considered such a feature, but not as a global flag. The current ts-auto-guard:type-guard is meant to leave room for other types of functions instead of type-guard. For example this could be ts-auto-guard:validator. I know the "export-all" functionality kind of clashes with this... It could be updated to export-all:validator,type-guard or something.

Either way I don't want to just have a flag for how the entire thing works because debug is meant for debugging only so it makes sense... A flag to switch your entire code base means that you can't choose between both!

So, this is my proposal:

/** 
 * @see {isFoo} ts-auto-guard:type-guard
 * @see {validateFoo} ts-auto-guard:validator
 **/
 interface Foo { /* ... */ }

Sounds reasonable, although i actually think that the --debug flag falls in the same category.
As of now i use both the original type-guard and my new function (validator is an appropriate term), since both are being created. I use it in a project, where i get non-typesafe data from a NoSQL database - not in production, but in an adminpanel to check for irregularities, especially during development, where data structures often are changed.

@rhys-vdw
Copy link
Owner

Hey @wiegell somehow I missed the notification that you responded. Sorry bout that.

Sounds reasonable, although i actually think that the --debug flag falls in the same category.

The --debug flag is there to work out why your type guards are failing, it's not intended to reshape the codebase. It's not intended to be used in production. In practise I created it to work out why one our giant Redux store initializer was returning false.

However a user could use a validator in its place for debugging so I'd actually be happy to remove the debug mode and instead give the option to create a validator which they can manually switch out for the failing check.

): string | null {
const { debug } = options
const propertyName = property.name

const isIdentifier = propertyName[0] !== '"'
const varName = isIdentifier
? `${objName}.${propertyName}`
: `${objName}[${propertyName}]`
? `${objName}?.${propertyName}`
Copy link
Owner

Choose a reason for hiding this comment

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

This ? shouldn't be necessary because if the object is undefined then it should be checked before the code attempts to access a property.

@rhys-vdw
Copy link
Owner

I'm def open to getting this merged if you're interested in working on it more. 👍

@wiegell
Copy link
Author

wiegell commented Jan 18, 2021

I have some other stuff going on atm., and my fork satisfies my current needs.
I understand your reasons, but it would be another major rework to completely separate the validator and debug-functionality and incorporate the new @see - though that indeed seems like the most appropriate way to do it.
Anyway it won't be anytime soon, you're welcome to close the PR or let it stay open as you best see fit.

@rhys-vdw
Copy link
Owner

@wiegell I'll leave it open as a reference. tbh this project is just running in the background for me, but someone might want to pick it up later.

@rhys-vdw rhys-vdw added the enhancement New feature or request label Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants