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

[API Proposal]: Make RegexRunner.CharInClass public instead of protected #84783

Closed
stephentoub opened this issue Apr 13, 2023 · 2 comments · Fixed by #84814
Closed

[API Proposal]: Make RegexRunner.CharInClass public instead of protected #84783

stephentoub opened this issue Apr 13, 2023 · 2 comments · Fixed by #84814
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.RegularExpressions
Milestone

Comments

@stephentoub
Copy link
Member

Background and motivation

The RegexRunner class is the public extensibility point via which source-generated regexes plug in (and via which the older CompileToAssembly compiled regexes plugged in). RegexRunner has a "CharInClass" method that's used by the customized implementations to check whether a character is in a given character class (stored in a particular string representation of that character class). This method is currently protected. But the source generator emits some helpers that want access to this method, which forces it to create a type derived from RegexRunner just to expose that member:

        internal class Base : RegexRunner
        {
            internal static new bool CharInClass(char ch, string charClass) => RegexRunner.CharInClass(ch, charClass);
        }

We can instead just make the member public.

API Proposal

namespace System.Text.RegularExpressions;

public abstract class RegexRunner
{
-    protected static bool CharInClass(char ch, string charClass);
+    public    static bool CharInClass(char ch, string charClass);
}

API Usage

n/a

Alternative Designs

No response

Risks

No response

@stephentoub stephentoub added area-System.Text.RegularExpressions api-ready-for-review API is ready for review, it is NOT ready for implementation labels Apr 13, 2023
@stephentoub stephentoub added this to the 8.0.0 milestone Apr 13, 2023
@ghost
Copy link

ghost commented Apr 13, 2023

Tagging subscribers to this area: @dotnet/area-system-text-regularexpressions
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

The RegexRunner class is the public extensibility point via which source-generated regexes plug in (and via which the older CompileToAssembly compiled regexes plugged in). RegexRunner has a "CharInClass" method that's used by the customized implementations to check whether a character is in a given character class (stored in a particular string representation of that character class). This method is currently protected. But the source generator emits some helpers that want access to this method, which forces it to create a type derived from RegexRunner just to expose that member:

        internal class Base : RegexRunner
        {
            internal static new bool CharInClass(char ch, string charClass) => RegexRunner.CharInClass(ch, charClass);
        }

We can instead just make the member public.

API Proposal

namespace System.Text.RegularExpressions;

public abstract class RegexRunner
{
-    protected static bool CharInClass(char ch, string charClass);
+    public    static bool CharInClass(char ch, string charClass);
}

API Usage

n/a

Alternative Designs

No response

Risks

No response

Author: stephentoub
Assignees: -
Labels:

area-System.Text.RegularExpressions, api-ready-for-review

Milestone: 8.0.0

@bartonjs
Copy link
Member

bartonjs commented Apr 13, 2023

Video

Looks good as proposed, though there was a recommendation to EditorBrowsable-Never the member (or the type, for that matter).

namespace System.Text.RegularExpressions;

public abstract class RegexRunner
{
-    protected static bool CharInClass(char ch, string charClass);
+    public    static bool CharInClass(char ch, string charClass);
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Apr 13, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 14, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Apr 14, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.RegularExpressions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants