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

Analysis context glossary entry #6239

Closed
wants to merge 3 commits into from
Closed

Conversation

MaryaBelanger
Copy link
Contributor

@MaryaBelanger MaryaBelanger commented Nov 26, 2024

Fixes #6145 (along with changes made in #6117, specifically this commit: d017546)

I took the definition from here, but I'm open to improvements if anyone has ideas

@dart-github-bot
Copy link
Collaborator

dart-github-bot commented Nov 26, 2024

Visit the preview URL for this PR (updated for commit 4a27c67):

https://dart-dev--pr6239-analysis-contexts-cx28iz7f.web.app

- term: "Analysis context"
short_description: |-
A representation of a set of code that should be analyzed using the same
sources of metadata.
Copy link
Member

Choose a reason for hiding this comment

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

I think that "sources of metadata" might not be accurate, or the best terms. For one thing, I think the only source of "metadata" is a package_config.json file, so "source of metadata" would be fine.

And at that point you might just use "metadata" or something...

I was going to suggest the word "configuration" like "source configuration" or "package configuration file", but (a) I think most users are not really familiar with the package config file, and might mix it up with an analysis options file, which at this point is quite distinct (and we took pains to separate them). So that's a little dicey. "Metadata" does nicely describe what distinguishes one set of sources from another, and doesn't require users to know about the package config files.

Copy link
Member

Choose a reason for hiding this comment

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

As I've said elsewhere, I think it's a mistake to add the term to the glossary at all. It's an implementation detail that I'd prefer for users to not need to be aware of. I would much rather have any existing documentation that uses the term be rewritten in such a way that it isn't used. For example, when discussing workspaces I think it would be better to describe the fact that using workspaces for large repositories will reduce the amount of memory used by the Dart language server and hence improve IDE performance without describing the implementation details that makes that statement true.

If I can't convince you to drop this PR (and I really hope I can), then I think it would be better to say "using the same package_config.json file.". The existence of this file is already mentioned in the documentation about pub, and I don't think it's unreasonable for users to be expected to understand the files being used to control compilation and analysis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR is very droppable, no worries there. The real reason for it was that I wanted to emphasize the performance improvements and was only thinking about that in terms of analysis contexts, so thought I had to define them somewhere. But the way you worded it:

the fact that using workspaces for large repositories will reduce the amount of memory used by the Dart language server and hence improve IDE performance

does a good job. Thanks for the useful information @srawlins and @bwilkerson! I will close this and change the reference in the new workspaces PR to not mention analysis contexts specifically, just the potential performance benefits.

A representation of a set of code that should be analyzed using the same
sources of metadata.
long_description: |-
An _analysis context_ is a representation of a set of code that should be
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I like "set of code." (Though I'm game to use consistent terms wherever we can, if it's a consistent term.)

In my mind this conjures an image of a pile of subroutines, like a 30-line function here and a 100-line class there.

One property that I think would be helpful for users is that the "set of code" is a set of files that are more-or-less co-located in the filesystem. So you have properties like:

  1. All of the files in one directory are always in the same analysis context.
  2. All of the files in one directory are in the same analysis context as the files in any subdirectories, unless there exists a new "metadata" file (.dart_tool/package_config.json). (Hard to write that one concisely and without explicitly introducing the package config file that we don't want users to worry about.)

In practice, as you look at a directory tree of source files, a new analysis context is introduced each time you see a pubspec.yaml file, as those generally pair with generated .dart_tool/package_config.yaml files. But if you haven't run dart pub get yet, that's not really true. And if any pubspec file is using the new Workspaces feature, it's not really true again.

After all that, I think the only suggestion I have is to use something like "set of Dart source files" rather than "set of code." Again, consistency with other text being important.

Copy link
Member

Choose a reason for hiding this comment

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

... without explicitly introducing the package config file that we don't want users to worry about.

I disagree. I think it's important for users to understand the role of the package config file and its relationship to a pubspec.yaml file (or potentially multiple pubspec.yaml files in the case of workspaces. It's essential to understanding how absolute URIs in directives are resolved.

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.

Document "analysis context" and account for workspaces too
4 participants