Skip to content

Clean up obsolete types in Razor #28418

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

Closed
wants to merge 2 commits into from
Closed

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia commented Dec 4, 2020

Part of #27529.

Summary of Changes

  • Remove IRazorEngineBuilder references and tests since it has been obsoleted for a while.
    • Didn't remove interface because RazorEngine takes dependency on it.
  • Obsoleted RazorEngine type.
    • We were planning on removing this type but it looks like our tests rely on it extensively. If we were to remove it we'd have to do an extensive refactor of the tests.
  • Un-obsolete GetItem since it was recently obsoleted.

@Pilchie Pilchie added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Dec 4, 2020
@ajaybhargavb
Copy link
Contributor

Did not remove obsolete IRazorCodeGenerationOptionsFeature or IRazorParserOptionsFeature. Will removing these cause issues with older Razor engines?

@NTaylorMullen, do you recall whether we still use these for anything or can they be safely removed? Context here

Did not remove obsolete GetItem since it was recently obsoleted.

@NTaylorMullen, this was obsoleted around April 2019. Can we safely remove it now but do you think we should wait for another release? I'm not sure if this is something customers will be depending on.

@NTaylorMullen
Copy link

@NTaylorMullen, do you recall whether we still use these for anything or can they be safely removed? Context here

Hmmm you can validate to see if they're consumed at all in the Mvc 1_x / 2_x projects: https://github.com/dotnet/aspnetcore/tree/master/src/Razor/Microsoft.AspNetCore.Mvc.Razor.Extensions.Version1_X

@NTaylorMullen, this was obsoleted around April 2019. Can we safely remove it now but do you think we should wait for another release? I'm not sure if this is something customers will be depending on.

Actually I think I was stupid for obsoleting this at all. I'd actually say it's probably worth "unobsoleting" it. There's still value.

@captainsafia
Copy link
Member Author

Actually I think I was stupid for obsoleting this at all. I'd actually say it's probably worth "unobsoleting" it. There's still value.

Fair point. Un-obsoleted.

Hmmm you can validate to see if they're consumed at all in the Mvc 1_x / 2_x projects: https://github.com/dotnet/aspnetcore/tree/master/src/Razor/Microsoft.AspNetCore.Mvc.Razor.Extensions.Version1_X

There's not consumed there. Just the Razor tests. I guess that means safe to remove?

@NTaylorMullen
Copy link

There's not consumed there. Just the Razor tests. I guess that means safe to remove?

Yup

@captainsafia captainsafia marked this pull request as ready for review December 9, 2020 02:49
@captainsafia captainsafia added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Dec 9, 2020
@ghost
Copy link

ghost commented Dec 9, 2020

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@pranavkm
Copy link
Contributor

pranavkm commented Dec 14, 2020

  • Leave the extension methods alone if we aren't removing the interfaces. Feel free to obsolete the extension methods and consider removing them post 6.0. In the obsoleting messages, point to the IRazorProjectEngine equivalent.
  • Feel free to remove everything from the 1_x and 2_x projects since they are tooling only.
  • Update sources that reference IRazorEngineFeature to use IRazorProjectEngineFeature

@captainsafia
Copy link
Member Author

Closing in favor of #28706.

@ghost
Copy link

ghost commented Dec 17, 2020

Hi @captainsafia. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@captainsafia captainsafia deleted the obsoletion/razor branch December 17, 2020 19:38
@mkArtakMSFT mkArtakMSFT removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants