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

retain postfix node in type section typed AST, with docgen fix #23101

Merged
merged 7 commits into from
Dec 23, 2023

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Dec 19, 2023

Continued from #23096 which was reverted due to breaking a package and failing docgen tests. Docgen should now work, but a PR is still pending for the package: SciNim/Unchained#45 has been merged

@metagn
Copy link
Collaborator Author

metagn commented Dec 21, 2023

CI now passes

@Araq
Copy link
Member

Araq commented Dec 22, 2023

Superb work, but how hard would it be for the doc gen to not render the export markers? Only public symbols are visible in the docs anyway so the export marker is redundant and it's bit uglier with them.

@metagn
Copy link
Collaborator Author

metagn commented Dec 22, 2023

Apparently the docgen just renders the AST, we could add a TRenderFlag that never renders postfixes. This would also make object fields not render postfixes anymore (right now they do). Edit: Added renderNoPostfix, but it has an exception for fields

That being said I only now realized let/const/proc also do not retain postfix nodes. I can do these as well but they have a chance of breaking packages again (especially proc), should they go in a separate PR after this one?

@Araq
Copy link
Member

Araq commented Dec 23, 2023

should they go in a separate PR after this one?

Yes, please.

@Araq Araq merged commit c0acf3c into nim-lang:devel Dec 23, 2023
17 of 19 checks passed
Copy link
Contributor

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from c0acf3c

Hint: mm: orc; opt: speed; options: -d:release
177542 lines; 7.825s; 768.309MiB peakmem

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