Skip to content

Determine file extensions on a per-package basis #2134

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

Merged
merged 3 commits into from
Jan 22, 2020

Conversation

jdkoren
Copy link
Contributor

@jdkoren jdkoren commented Jan 17, 2020

We need a way to change extensions for generated files. Some considerations:

Generators can emit additional files, so having the generator apply/change file extensions for everything isn't ideal. We can store something in the PackageGraph and look it up in our models, but polluting that class is a bit of a code smell--however, we do that already for the RendererFactory, so I opted to use that existing pattern for this purpose. Happy to explore alternatives.

@googlebot googlebot added the cla: yes Google CLA check succeeded. label Jan 17, 2020
@jdkoren jdkoren requested a review from jcollins-g January 17, 2020 21:16
@jcollins-g
Copy link
Contributor

I just realized something -- that with --link-to-remote we're making an implicit assumption about how other packages are rendered and that it is the same as what we're rendering locally.

That should be possible to fix by adding some data to the table that knows how to link to remote packages. That might influence how we decide to determine the extension -- perhaps, it should be owned by the Package class and derived directly rather than indirectly through the renderer.

Copy link
Contributor

@jcollins-g jcollins-g left a comment

Choose a reason for hiding this comment

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

Filename rendering needs to take into account --link-to-remote and whether or not remote repositories are rendered with HTML.

@jdkoren
Copy link
Contributor Author

jdkoren commented Jan 21, 2020

Changed so that Package determines the file type and other classes defer to that. Currently it does a simple check on documentedWhere as we suspect the vast majority of cases to have remote docs in html format.

Also rebased.

@jdkoren jdkoren requested a review from jcollins-g January 21, 2020 19:11
if (package.documentedWhere == DocumentLocation.remote) {
return 'html';
}
return packageGraph.rendererFactory.fileTypeRenderer.fileType;
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'm contemplating using the config member to lookup the format instead of using a renderer here. That requires moving format out of GeneratorContext and into DartdocOptionContext.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. It's not immediately clear to me which is preferable, but a config member would be the most direct way to get this information. I'm OK with either choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to using the config member as it's a bit more direct.

@jdkoren jdkoren changed the title Extract file extensions to a new renderer Determine file extensions on a per-package basis Jan 22, 2020
@jdkoren jdkoren merged commit 4cad5ad into dart-lang:master Jan 22, 2020
@jdkoren jdkoren mentioned this pull request Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA check succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants