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

Tackle multiple instances of a TypeName within a schema #667

Draft
wants to merge 15 commits into
base: next
Choose a base branch
from

Conversation

sramam
Copy link
Contributor

@sramam sramam commented Feb 17, 2021

This is an implementation for resolving #666, valid TypeScript programs that the schema-generator cannot handle due to "multiple-instances" in schema error.

@lgtm-com
Copy link

lgtm-com bot commented Feb 17, 2021

This pull request introduces 1 alert when merging 9b34f66 into 6c6f9c9 - view on LGTM.com

new alerts:

  • 1 for Unreachable statement

@sramam
Copy link
Contributor Author

sramam commented Feb 17, 2021

@domoritz, I realize my PRs have probably disrupted your week. This is the last one I promise.

There is still a failing test case - related to the def- prefix attached to DefinitionTypes.
I'm working on that, but the rest of the change should be reviewable for substance - not form.

I'd appreciate your time in reviewing both the issue #666 and this change and providing directional guidance.

@domoritz domoritz marked this pull request as draft February 17, 2021 13:29
@lgtm-com
Copy link

lgtm-com bot commented Feb 20, 2021

This pull request introduces 3 alerts when merging fa9b8b5 into ed725fc - view on LGTM.com

new alerts:

  • 3 for Unused variable, import, function or class

@sramam sramam changed the title [WIP] proof of concept for #666 Tackle multiple instances of a TypeName within a schema Feb 20, 2021
@sramam
Copy link
Contributor Author

sramam commented Feb 20, 2021

@domoritz, this change should be good to go

  • all test cases pass,
  • no regressions are added to vega-lite.

Coverage does fall. #671/#672 is an attempt to tackle some of that.

@sramam sramam marked this pull request as ready for review March 7, 2021 02:00
@domoritz domoritz self-requested a review April 4, 2021 18:09
Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

Thank you for the Pull request. It looks like it was a lot of work so I appreciate the work you have done. Unfortunately, I am a bit overwhelmed by the size of the changes. Is there any way to break this up, reduce the complexity or at least increase test coverage?

One change I can see for example is to make srcFileName a property of base type and in the derived classes set it in super. Then you would definitely have better test coverage already since a lot of the untested code is in getSrcFileName.

@sramam
Copy link
Contributor Author

sramam commented Apr 5, 2021

Thanks for taking the time to review.

I've had to move on to other things - so I might be a bit slow to get this done.
However I'd like nothing more than not having to maintain this in a fork for my personal use.
So the first chance I get, I will be working on getting this merged.

I like the getSrcFileName idea. While I don't like the large change either, I am a bit skeptical it can be done piecemeal.
That said, I promise to keep an open mind!

@domoritz
Copy link
Member

domoritz commented Apr 5, 2021

Thanks! I am happy to help with more reviews. I'll mark this pull request as draft for now.

@domoritz domoritz marked this pull request as draft April 5, 2021 16:11
@domoritz
Copy link
Member

@sramam can you finish up this pull request?

@sramam
Copy link
Contributor Author

sramam commented Jun 30, 2021

@sramam can you finish up this pull request?

I haven't found the time to get into making the changes you suggested. The context switch time is much larger for me and this would require a large block of time which has been a hurdle.

It's on my back-burner though - in the sense that I feel guilty not having closed this out. Hopefully soon.

@domoritz
Copy link
Member

No need to feel guilty. I appreciate that you want to make the time. Personal care should take priority over this project.

@domoritz domoritz changed the base branch from master to next October 5, 2021 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants