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

feat: expose resolved document #433

Merged
merged 5 commits into from
Aug 20, 2019
Merged

feat: expose resolved document #433

merged 5 commits into from
Aug 20, 2019

Conversation

P0lip
Copy link
Contributor

@P0lip P0lip commented Aug 11, 2019

Closes #398

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

We haven't settled on any final implementation just yet, therefore the following PR is more about exposing a place to discuss the final solution.
I know we could do it in the related GitHub issue, but having everything shown in the code should make it easier to reason about.

@nulltoken looking forward to your input! :)

In general, there are a couple of other ways to accomplish that. I'm afraid that all of them have some drawbacks. I'll try to list 3 other solutions I can think of.
Note, all the concepts would result in non-breaking functionality.

  • have a resolved public property that whose value would assigned when run method is executed
  • onResolve callback somewhere in run function, most likely right after resolving is completed
  • set a symbol / non-enumerable on results array and expose a method on Spectral instance to retrieve the previously set document

As you can see, all of them are kind of weird.
Ideally, run method would always return some metadata next to validation results, but this would be breaking.

@P0lip P0lip self-assigned this Aug 11, 2019
@P0lip P0lip added the enhancement New feature or request label Aug 11, 2019
@P0lip P0lip added this to the September '19 milestone Aug 11, 2019
src/spectral.ts Outdated
@@ -42,7 +43,18 @@ export class Spectral {
this._resolver = opts && opts.resolver ? opts.resolver : new Resolver();
}

public async run(target: IParsedResult | object | string, opts: IRunOpts = {}): Promise<IRuleResult[]> {
public async run(
Copy link
Contributor

Choose a reason for hiding this comment

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

Whaaaaaat is this. Does TypeScript have overloading?

I have a funny feeling in my stomach, and I don't think it's just because of the North Sea. Did you consider alternate methods which can be called to return the resolved doc and results?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, all of them have some pros and cons.
For me, the most reasonable one would be to have getResolved method, but the gotcha here is that one would need to call run beforehand.

@philsturgeon
Copy link
Contributor

This is going to have conflicts with #430, I think we should focus on that one then come back to this?

@P0lip
Copy link
Contributor Author

P0lip commented Aug 19, 2019

@philsturgeon
Up to you folks. Resolving potential conflicts will be very straightforward, so we can safely wait until #430 is merged.

What do you all think about the potential solutions? That run with those overloads seems weird.
I'd be happy to make a breaking change and just always return such an object

data: {}, // resolved
results: [], // validation results

This would let us add more data if we'd ever need to.
Unfortunately, we are not releasing 5.0 but 4.1, so this move might be impossible this time.

@philsturgeon
Copy link
Contributor

This is a tricky one for sure. How about a 2nd runWithResolved() method which always returns both, then in 5.0 we just rename it and run() returns both all the time.

I prefer this to overloading and "argument changes response type" style code.

@P0lip
Copy link
Contributor Author

P0lip commented Aug 19, 2019

runWithResolved() sounds like a good trade-off to me 👍

@P0lip P0lip merged commit a92d456 into develop Aug 20, 2019
@P0lip P0lip deleted the feat/expose-resolved branch August 20, 2019 14:32
philsturgeon pushed a commit that referenced this pull request Aug 22, 2019
* feat: expose resolved document

* refactor: runWithResolved
P0lip added a commit that referenced this pull request Aug 29, 2019
* feat: expose resolved document

* refactor: runWithResolved
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.

Expose to the caller of .run() the fully resolved result
2 participants