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

Use top_level_semantic in doc spec instead of semantic #9352

Merged

Conversation

makenowjust
Copy link
Contributor

This PR adds top_level_semantic utility methods for compiler spec, and uses this in doc spec. crystal docs command uses top_level_semantic, so the spec follows it.

Copy link
Contributor

@nobodywasishere nobodywasishere left a comment

Choose a reason for hiding this comment

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

Everything looks good to me - though I don't have write access so I cannot merge this PR. Does this generate the exact same docs as just 'semantic'? And how kuch faster is this?

@straight-shoota
Copy link
Member

The docs generator only needs and uses top level semantic, so the result should be entirely correct.

@straight-shoota
Copy link
Member

I'm not sure about the wants_doc = true default because its different from anywhere else.
I suppose it makes kind of sense when it's only used for doc generator specs.
However then I'd maybe consider extracing a specific helper method for that and maybe returns a Doc::Generator instance?

nobodywasishere

This comment was marked as duplicate.

@@ -77,7 +77,23 @@ def semantic(node : ASTNode, *, warnings = nil, wants_doc = false, flags = nil)
SemanticResult.new(program, node)
end

def assert_normalize(from, to, flags = nil, *, file = __FILE__, line = __LINE__)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be removed, it's why CI is failing

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

Specs are green now 🚀

spec/spec_helper.cr Outdated Show resolved Hide resolved
spec/spec_helper.cr Outdated Show resolved Hide resolved
@straight-shoota straight-shoota self-assigned this Dec 12, 2023
@straight-shoota straight-shoota added this to the 1.11.0 milestone Dec 12, 2023
@straight-shoota straight-shoota changed the title refactor: use top_level_semantic in doc spec instead of semantic Use top_level_semantic in doc spec instead of semantic Dec 13, 2023
@straight-shoota straight-shoota merged commit 59095ff into crystal-lang:master Dec 13, 2023
55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants