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 scope and isStatic to QualifiedName, as well as parsing and a URI format #101

Merged
merged 2 commits into from
Oct 21, 2024

Conversation

jakemac53
Copy link
Contributor

Closes #96, I think for libraries we will want just a Uri essentially, since they aren't first class objects in Dart, you can't refer directly to them.

@jakemac53 jakemac53 requested a review from davidmorgan October 11, 2024 18:43
Copy link
Contributor

@davidmorgan davidmorgan left a comment

Choose a reason for hiding this comment

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

Looks good, thanks :)

Property('name',
type: 'String',
description: 'The name of the declaration to look up.'),
Property('isStatic',
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this will probably end up as an enum, since we might want to distinguish more types of thing (e.g. getter vs setter vs field), and more might arrive later. Maybe a TODO since it's easy to change for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it ever matters whether it is a getter/setter/field for the purposes of this object, it is just telling you how to look up a declaration unambiguously, which does require knowing if it should be looked up in the static or instance member scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't matter for right now, but I was thinking of a case where you have e.g. a subclass with a setter and superclass with a getter; you need to say setter or getter to disambiguate between the two. It's a bit of an awkward corner :) anyway, we can figure that out later if needed. Thanks.

Copy link
Contributor Author

@jakemac53 jakemac53 Oct 22, 2024

Choose a reason for hiding this comment

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

Oh for setters, the name should probably be followed by an =. At least that is how most of the other tooling deals with this afaik. cc @johnniwinther @scheglov for comment.

Although in terms of macros, maybe we do want to distinguish here, because we don't really want to include the = when outputting code. So I think the existing macro stuff does not include it.

jakemac53 added a commit that referenced this pull request Oct 21, 2024
Closes #100

- Adds `model` field to AugmentRequest, with implementations to pass it (although, these use a host side query).
  - Expose the `hostService` from the `MacroHost` (used to do the query)
- Adds a `Model` parameter to all macro methods (typed as `Object` for right now for most of them, as we don't have better types yet).
- Make all fields on AugmentRequest required (this simplified implementations, but lmk if you want me to revert this).
- Rename the `Model.qualifiedNameOfMember` extension to `qualifiedNameOf` generalizing and fixing it:
  -  Now it actually does what it says 🤣 (it used to return just the qualified name of the type the member is declared in I think?), there was in any case no way to represent members with qualified names, see #101 which adds that.
  - It now throws for anything other than types, since they can't be referenced
  - It just accepts a `Map<String, Object?>` now which is the only thing we can generalize on for the moment, but see #103 for one possible workaround.
- Fixes up tests and removes expected request/response from the client for the model
@jakemac53 jakemac53 merged commit 44bc307 into main Oct 21, 2024
45 checks passed
@jakemac53 jakemac53 deleted the qualified-name branch October 21, 2024 16:39
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.

QualifiedName should be able to reference top level members of libraries and members of interfaces
2 participants