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

Implement annotated source-view & make default #356

Merged
merged 12 commits into from
Mar 8, 2023
Merged

Conversation

timholy
Copy link
Member

@timholy timholy commented Mar 2, 2023

This leverages TypedSyntax to attach type-annotations to user source code (see the README on this branch for an overview). This is intended as a more "newbie-friendly" mode for Cthulhu. Because source text is typically less verbose than IRCode, it may even be useful in some circumstances for Cthulhu veterans.

This is the second half of #345. Tests pass locally but will not pass here until JuliaRegistries/General#78815 merges. But I thought it would be better to post this now to give folks time to review these changes.

Closes #345.

This leverages TypedSyntax to attach type-annotations to user
source code. This is intended as a more "newbie-friendly"
mode for Cthulhu. Because source text is typically less
verbose than IRCode, it may even be useful in some circumstances
for Cthulhu veterans.
@timholy
Copy link
Member Author

timholy commented Mar 5, 2023

TypedSyntax is now registered. Let's re-run the tests.

@timholy timholy closed this Mar 5, 2023
@timholy timholy reopened this Mar 5, 2023
@timholy timholy force-pushed the teh/sourcetext2 branch 8 times, most recently from fe1461a to d09a696 Compare March 6, 2023 14:01
@timholy
Copy link
Member Author

timholy commented Mar 6, 2023

I think this is ready for review; the integration test failure may require changes in SnoopCompile and is something that can be addressed later. (EDIT: failure also appeared in #357 so presumably unrelated.) Thanks @fredrikekre for the tips about how to fix CI.

I decided it might be better to make coordinated development between TypedSyntax & Cthulhu "easy," so CI devs TypedSyntax rather than relying on the registered version. To me that's the main advantage of subdir packages, but there's a gotcha. I added a CONTRIBUTING.md document to describe the gotcha, and best practice for avoiding it. Please read it carefully.

I do hope to merge & register this soon; I've been delaying making a video that demonstrates the new version until this is "final" (I now have too many followers on Youtube to put up "junk" videos), but some students in my class have conflicts and can't attend in person. So I'm feeling some pressure to get this finished.

@aviatesk
Copy link
Member

aviatesk commented Mar 6, 2023

I do hope to merge & register this soon; I've been delaying making a video that demonstrates the new version until this is "final" (I now have too many followers on Youtube to put up "junk" videos), but some students in my class have conflicts and can't attend in person. So I'm feeling some pressure to get this finished.

Thanks for working on this. I might be unable to give a full review on this soon, but I'm fine with merging this as is and revising it if necessary.

@timholy
Copy link
Member Author

timholy commented Mar 6, 2023

You've already given this a spin, I know, and I appreciate it! And implementation details can always be fixed later, so if you're good with it then I think that makes sense.

For anyone who hasn't tested this yet and who may understandably be a bit worried about it, perhaps the best option is to just try the branch out with, e.g., the demo in the README. That will let you get a better understanding of what the user experience is like and whether it's smooth to switch back to traditional IRCode/CodeInfo views. I won't merge until I've played with that again myself, but any other eyeballs (and especially, real-world usage, which I've done very little of so far) are always appreciated.

@timholy timholy force-pushed the teh/sourcetext2 branch 2 times, most recently from 952a1a8 to 93b8777 Compare March 6, 2023 21:07
@timholy
Copy link
Member Author

timholy commented Mar 6, 2023

Whew, no 1.7 failures.

timholy added 3 commits March 7, 2023 12:19
This is handy for people who have customized default values,
or who are disabling precompilation.
@timholy
Copy link
Member Author

timholy commented Mar 7, 2023

I've done much more real-world usage of this and handled a wider variety of code. Unless objections arise, I'll merge & register tomorrow. I think Cthulhu 2.8.0 is a good version number for this, but if anyone would prefer to go to 3.0.0, let me know.

@timholy timholy merged commit ea0d3b2 into master Mar 8, 2023
@timholy timholy deleted the teh/sourcetext2 branch March 8, 2023 12:48
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