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

Add requiredXYZ symbols to TASTy reflect context #7903

Conversation

@nicolasstucki
Copy link
Contributor

nicolasstucki commented Jan 6, 2020

No description provided.

@nicolasstucki nicolasstucki requested a review from liufengyun Jan 6, 2020
@nicolasstucki nicolasstucki self-assigned this Jan 6, 2020
@nicolasstucki nicolasstucki marked this pull request as ready for review Jan 6, 2020
Copy link
Contributor

liufengyun left a comment

Otherwise, LGTM

def Context_requiredClass(self: Context)(path: String): Symbol

/** Get module symbol if module is either defined in current compilation run or present on classpath. */
def Context_requiredModule(self: Context)(path: String): Symbol

This comment has been minimized.

Copy link
@liufengyun

liufengyun Jan 7, 2020

Contributor

The meta-programmer may not know what's a module. Maybe rename to requiredObject?

This comment has been minimized.

Copy link
@nicolasstucki

nicolasstucki Jan 7, 2020

Author Contributor

Better to keep names consistent. It will be simpler for documentation. We also have moduleClass and companionModule that use the same naming scheme.

This comment has been minimized.

Copy link
@liufengyun

liufengyun Jan 7, 2020

Contributor

I totally agree, for advanced meta-programming, keeping the concepts closer to the compiler will help both meta-programmers who want to get into the compiler and maintenance of the framework.

@nicolasstucki nicolasstucki merged commit a8ea206 into lampepfl:master Jan 7, 2020
2 checks passed
2 checks passed
CLA User signed CLA
Details
continuous-integration/drone/pr Build is passing
Details
@nicolasstucki nicolasstucki deleted the dotty-staging:add-required-symbols-to-tasty-reflect branch Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.