Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should fix your issue, while leaving my original fix intact
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my original PR, that equality check was still wrong, it just didn't surface until the move to optional chaining
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably misunderstood your question
The second render works in the following cases:
but not in case the equality is not reversed:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok, hmm... I'll have to take another look at this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ccatterina , sorry for the late reply - Covid finally made it into our home, so I've been super busy with a sick and preggo wife, and a sick toddler.
If I'm understanding you correctly, the neg type check is so that we're casting anything that behaves like an
enum
into one? I wonder if adding a small docblock would help clear this up in the future.I dug into some of these other resources, and it looks like there's specifically an
@enum
directive that isn't implemented in vuedocgen.@valentinpalkovic I think I'm going to resolve this conversation as I think the proposed solution is the way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that you two (seemingly smart people!) have discussed how this single character make it all behave, I think it makes sense to refactor the code to be more readable for future work. Either by making the
matched
variable more verbose, or by adding comments describing what the conditions do, and why.I don't know much about Vue so I trust that you two have got it under control, but the next person is probably going to be just as confused as you initially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I think that the most confusing thing is the name of the function
isEnum
, maybe something likeinferEnum
comes out clearer.I could rename the function and a comment to describe its purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JReinhold @ProjektGopher I pushed a new commit that tries to make the purpose of the function clearer, feel free to make some changes (my english is not so good).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this does a decent job explaining the situation. I feel like it's good to go