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

Simplify ASR.asdl #3063

Merged
merged 9 commits into from
Jan 11, 2024
Merged

Simplify ASR.asdl #3063

merged 9 commits into from
Jan 11, 2024

Conversation

khushi-411
Copy link
Contributor

@khushi-411 khushi-411 commented Jan 9, 2024

Fixes: #1900

TODO

  • add doccumentation

@khushi-411 khushi-411 marked this pull request as ready for review January 9, 2024 12:22
@khushi-411 khushi-411 marked this pull request as draft January 9, 2024 12:23
@certik
Copy link
Contributor

certik commented Jan 9, 2024

I think this looks good so far.

@certik
Copy link
Contributor

certik commented Jan 9, 2024

We can move all the comments into doc/src/asr/asr.md, and just ensure they are all there. Then we can merge this PR.

After that, we can iteratively improve the documentation to move things around (split into individual nodes) and improve.

src/libasr/ASR.asdl Outdated Show resolved Hide resolved
@khushi-411 khushi-411 marked this pull request as ready for review January 10, 2024 06:29
Copy link
Member

@Thirumalai-Shaktivel Thirumalai-Shaktivel left a comment

Choose a reason for hiding this comment

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

Rest looks good, thank you!

doc/src/asr/asr.md Outdated Show resolved Hide resolved
doc/src/asr/asr.md Outdated Show resolved Hide resolved
doc/src/asr/asr.md Outdated Show resolved Hide resolved
doc/src/asr/asr.md Outdated Show resolved Hide resolved
doc/src/asr/asr.md Outdated Show resolved Hide resolved
doc/src/asr/asr.md Outdated Show resolved Hide resolved
doc/src/asr/asr.md Show resolved Hide resolved
doc/src/asr/asr.md Outdated Show resolved Hide resolved
doc/src/asr/asr.md Outdated Show resolved Hide resolved
doc/src/asr/asr.md Outdated Show resolved Hide resolved
src/libasr/ASR.asdl Outdated Show resolved Hide resolved
src/libasr/ASR.asdl Outdated Show resolved Hide resolved
doc/src/asr/asr.md Outdated Show resolved Hide resolved
@certik
Copy link
Contributor

certik commented Jan 10, 2024

I think this looks great. Fix up the conflicts and the few things I noticed, then we can merge and improve upon it later.

@certik certik marked this pull request as draft January 10, 2024 19:59
@khushi-411
Copy link
Contributor Author

Done! Thank you for all your prompt reviews in this PR :-)

@certik certik marked this pull request as ready for review January 11, 2024 00:27
@certik
Copy link
Contributor

certik commented Jan 11, 2024

Thanks. One last thing: #3063 (comment).

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

I think it looks good now. Now all comments were ported, but most of them were, so I think this is good enough to merge. Thanks!

@certik certik merged commit 71637ca into lfortran:main Jan 11, 2024
21 checks passed
@khushi-411 khushi-411 deleted the simplify_asdl branch January 11, 2024 06:33
Kishan-Ved pushed a commit to Kishan-Ved/lfortran that referenced this pull request Jan 11, 2024
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.

Simplify ASR.asdl
3 participants