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

Port to evaldown #54

Merged
merged 10 commits into from
Jul 4, 2020
Merged

Port to evaldown #54

merged 10 commits into from
Jul 4, 2020

Conversation

alexjeffburke
Copy link
Member

@alexjeffburke alexjeffburke commented May 2, 2020

Rewrite unexpected-markdown to evaluate snippets via evaldown.

As evaldown evolved problems arose where this module was at odds
with the semantics supported by the newer runtime. The root cause
was that a duplicate runtime effectively existed in this module which
could then get out of sync etc.

Opt to sidestep this situation going forward by directly evaluating
snippets using evaldown and having this module responsible for
asserting on the result of attempting to do so only.

This commit also makes node 10 the minimum supported version.
As evaldown evolved and the docs started to rely on its new behaviour
problems arose with unexpected-markdown.In particular, this module
attempted to replicate the behaviour of snippet evaluation and was
effectively a separate implementation of it which meant it became
out of sync.

In order to avoid this situation going forward, rewrite the module
to make use of the same runtime and merely assert that afterwards
the evaluated result matches the state of the on-disk markdown file.
@alexjeffburke alexjeffburke marked this pull request as ready for review June 28, 2020 11:16
@alexjeffburke
Copy link
Member Author

The biggest changes here are to the convertMarkdownToMocha file (https://github.com/unexpectedjs/unexpected-markdown/pull/54/files#diff-a666ff3aa411491e84b64207c5c4e20a) with the rest being the removal of the old runtime.

Copy link
Member

@papandreou papandreou left a comment

Choose a reason for hiding this comment

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

LGTM. I guess we're getting close to being able to ditch unexpected-markdown?

lib/convertMarkdownToMocha.js Outdated Show resolved Hide resolved
@alexjeffburke
Copy link
Member Author

LGTM. I guess we're getting close to being able to ditch unexpected-markdown?

Yeah, if we wanted to :) I will say that with the runtime no longer duplicated what's here should at least be far easier to maintain which is at least a plus in the short term.

I still really like the use-case supported by unexpected-markdown of easily determine whether there are any errors in the examples and be able to surface that information in a very consumable way on CI - but there may well be easier/better way of achieving that we should definitely look into.

@alexjeffburke alexjeffburke merged commit d997e84 into master Jul 4, 2020
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.

None yet

2 participants