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.

Sign up for GitHub

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

Jump to bottom

Remove PowerFxConfig.Culture #1292

Merged
merged 11 commits into from
Apr 4, 2023
Merged

Remove PowerFxConfig.Culture #1292

merged 11 commits into from
Apr 4, 2023

Conversation

LucGenetier
Copy link
Contributor

No description provided.

@LucGenetier
Copy link
Contributor Author

Public API changes:
❌CP0002 M:Microsoft.PowerFx.Engine.CreateFieldRenamer(Microsoft.PowerFx.Types.RecordType,Microsoft.PowerFx.Core.Utils.DPath,Microsoft.PowerFx.Core.Utils.DName)
❌CP0002 M:Microsoft.PowerFx.Engine.GetDisplayExpression(System.String,Microsoft.PowerFx.ReadOnlySymbolTable)
❌CP0002 M:Microsoft.PowerFx.Engine.GetDisplayExpression(System.String,Microsoft.PowerFx.Types.RecordType)
❌CP0002 M:Microsoft.PowerFx.Engine.GetInvariantExpression(System.String,Microsoft.PowerFx.Types.RecordType)
❌CP0002 M:Microsoft.PowerFx.Engine.Parse(System.String,Microsoft.PowerFx.Features,Microsoft.PowerFx.ParserOptions,System.Globalization.CultureInfo)
❌CP0002 M:Microsoft.PowerFx.Engine.Parse(System.String,Microsoft.PowerFx.ParserOptions,System.Globalization.CultureInfo)
❌CP0002 M:Microsoft.PowerFx.Engine.Tokenize(System.String)
❌CP0002 M:Microsoft.PowerFx.ParserOptions.#ctor
❌CP0002 M:Microsoft.PowerFx.PowerFxConfig.#ctor(System.Globalization.CultureInfo,Microsoft.PowerFx.Features)
❌CP0002 M:Microsoft.PowerFx.PowerFxConfig.#ctor(System.Globalization.CultureInfo)
❌CP0002 M:Microsoft.PowerFx.PowerFxConfig.get_CultureInfo

@LucGenetier
Copy link
Contributor Author

Public API changes:
❌CP0002 M:Microsoft.PowerFx.Engine.CreateFieldRenamer(Microsoft.PowerFx.Types.RecordType,Microsoft.PowerFx.Core.Utils.DPath,Microsoft.PowerFx.Core.Utils.DName)
❌CP0002 M:Microsoft.PowerFx.Engine.GetDisplayExpression(System.String,Microsoft.PowerFx.ReadOnlySymbolTable)
❌CP0002 M:Microsoft.PowerFx.Engine.GetDisplayExpression(System.String,Microsoft.PowerFx.Types.RecordType)
❌CP0002 M:Microsoft.PowerFx.Engine.GetInvariantExpression(System.String,Microsoft.PowerFx.Types.RecordType)
❌CP0002 M:Microsoft.PowerFx.Engine.Parse(System.String,Microsoft.PowerFx.Features,Microsoft.PowerFx.ParserOptions,System.Globalization.CultureInfo)
❌CP0002 M:Microsoft.PowerFx.Engine.Parse(System.String,Microsoft.PowerFx.ParserOptions,System.Globalization.CultureInfo)
❌CP0002 M:Microsoft.PowerFx.Engine.Tokenize(System.String)
❌CP0002 M:Microsoft.PowerFx.ParserOptions.#ctor
❌CP0002 M:Microsoft.PowerFx.PowerFxConfig.#ctor(System.Globalization.CultureInfo,Microsoft.PowerFx.Features)
❌CP0002 M:Microsoft.PowerFx.PowerFxConfig.#ctor(System.Globalization.CultureInfo)
❌CP0002 M:Microsoft.PowerFx.PowerFxConfig.get_CultureInfo

MikeStall
MikeStall reviewed Apr 3, 2023
View reviewed changes
src/libraries/Microsoft.PowerFx.Core/Parser/ParserOptions.cs
@@ -37,6 +37,13 @@ public class ParserOptions
/// It is an immediate parser error if the expression is too long.
/// </summary>
public int MaxExpressionLength { get; set; }

public ParserOptions(CultureInfo culture = null, bool allowsSideEffects = false, int maxExpressionLength = 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

ParserOptions

nit: adding a ctor (when there were none previously) is a breaking change because it removes the implicit default ctor.
Consider either:

  • not adding hte ctor (treat as a true poco)
  • add both a default ctor and additional ctor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As all params are optional, we already have a default ctor, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, not according to the .net loader.

A default ctor means no parameters.
Whereas a default parameter is a compiler-level construct (the compiler will insert the default value at the callsite), but it's still a regular parameter for .net loader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True but I don't think it matters here

@LucGenetier
Copy link
Contributor Author

Public API changes:
❌CP0002 M:Microsoft.PowerFx.Engine.CreateFieldRenamer(Microsoft.PowerFx.Types.RecordType,Microsoft.PowerFx.Core.Utils.DPath,Microsoft.PowerFx.Core.Utils.DName)
❌CP0002 M:Microsoft.PowerFx.Engine.GetDisplayExpression(System.String,Microsoft.PowerFx.ReadOnlySymbolTable)
❌CP0002 M:Microsoft.PowerFx.Engine.GetDisplayExpression(System.String,Microsoft.PowerFx.Types.RecordType)
❌CP0002 M:Microsoft.PowerFx.Engine.GetInvariantExpression(System.String,Microsoft.PowerFx.Types.RecordType)
❌CP0002 M:Microsoft.PowerFx.Engine.Parse(System.String,Microsoft.PowerFx.Features,Microsoft.PowerFx.ParserOptions,System.Globalization.CultureInfo)
❌CP0002 M:Microsoft.PowerFx.Engine.Parse(System.String,Microsoft.PowerFx.ParserOptions,System.Globalization.CultureInfo)
❌CP0002 M:Microsoft.PowerFx.Engine.Tokenize(System.String)
❌CP0002 M:Microsoft.PowerFx.ParserOptions.#ctor
❌CP0002 M:Microsoft.PowerFx.PowerFxConfig.#ctor(System.Globalization.CultureInfo,Microsoft.PowerFx.Features)
❌CP0002 M:Microsoft.PowerFx.PowerFxConfig.#ctor(System.Globalization.CultureInfo)
❌CP0002 M:Microsoft.PowerFx.PowerFxConfig.get_CultureInfo

@MikeStall MikeStall mentioned this pull request Apr 4, 2023
Closed
MikeStall
MikeStall previously approved these changes Apr 4, 2023
View reviewed changes
@LucGenetier
Copy link
Contributor Author

Public API changes:
❌CP0002 M:Microsoft.PowerFx.Engine.CreateFieldRenamer(Microsoft.PowerFx.Types.RecordType,Microsoft.PowerFx.Core.Utils.DPath,Microsoft.PowerFx.Core.Utils.DName)
❌CP0002 M:Microsoft.PowerFx.Engine.GetDisplayExpression(System.String,Microsoft.PowerFx.ReadOnlySymbolTable)
❌CP0002 M:Microsoft.PowerFx.Engine.GetDisplayExpression(System.String,Microsoft.PowerFx.Types.RecordType)
❌CP0002 M:Microsoft.PowerFx.Engine.GetInvariantExpression(System.String,Microsoft.PowerFx.Types.RecordType)
❌CP0002 M:Microsoft.PowerFx.Engine.Parse(System.String,Microsoft.PowerFx.Features,Microsoft.PowerFx.ParserOptions,System.Globalization.CultureInfo)
❌CP0002 M:Microsoft.PowerFx.Engine.Parse(System.String,Microsoft.PowerFx.ParserOptions,System.Globalization.CultureInfo)
❌CP0002 M:Microsoft.PowerFx.Engine.Tokenize(System.String)
❌CP0002 M:Microsoft.PowerFx.ParserOptions.#ctor
❌CP0002 M:Microsoft.PowerFx.PowerFxConfig.#ctor(System.Globalization.CultureInfo,Microsoft.PowerFx.Features)
❌CP0002 M:Microsoft.PowerFx.PowerFxConfig.#ctor(System.Globalization.CultureInfo)
❌CP0002 M:Microsoft.PowerFx.PowerFxConfig.get_CultureInfo

@LucGenetier
Copy link
Contributor Author

Public API changes:
❌CP0002 M:Microsoft.PowerFx.Engine.CreateFieldRenamer(Microsoft.PowerFx.Types.RecordType,Microsoft.PowerFx.Core.Utils.DPath,Microsoft.PowerFx.Core.Utils.DName)
❌CP0002 M:Microsoft.PowerFx.Engine.GetDisplayExpression(System.String,Microsoft.PowerFx.ReadOnlySymbolTable)
❌CP0002 M:Microsoft.PowerFx.Engine.GetDisplayExpression(System.String,Microsoft.PowerFx.Types.RecordType)
❌CP0002 M:Microsoft.PowerFx.Engine.GetInvariantExpression(System.String,Microsoft.PowerFx.Types.RecordType)
❌CP0002 M:Microsoft.PowerFx.Engine.Parse(System.String,Microsoft.PowerFx.Features,Microsoft.PowerFx.ParserOptions,System.Globalization.CultureInfo)
❌CP0002 M:Microsoft.PowerFx.Engine.Parse(System.String,Microsoft.PowerFx.ParserOptions,System.Globalization.CultureInfo)
❌CP0002 M:Microsoft.PowerFx.Engine.Tokenize(System.String)
❌CP0002 M:Microsoft.PowerFx.ParserOptions.#ctor
❌CP0002 M:Microsoft.PowerFx.PowerFxConfig.#ctor(System.Globalization.CultureInfo,Microsoft.PowerFx.Features)
❌CP0002 M:Microsoft.PowerFx.PowerFxConfig.#ctor(System.Globalization.CultureInfo)
❌CP0002 M:Microsoft.PowerFx.PowerFxConfig.get_CultureInfo

@LucGenetier
Copy link
Contributor Author

Public API changes:
❌CP0002 M:Microsoft.PowerFx.Engine.CreateFieldRenamer(Microsoft.PowerFx.Types.RecordType,Microsoft.PowerFx.Core.Utils.DPath,Microsoft.PowerFx.Core.Utils.DName)
❌CP0002 M:Microsoft.PowerFx.Engine.GetDisplayExpression(System.String,Microsoft.PowerFx.ReadOnlySymbolTable)
❌CP0002 M:Microsoft.PowerFx.Engine.GetDisplayExpression(System.String,Microsoft.PowerFx.Types.RecordType)
❌CP0002 M:Microsoft.PowerFx.Engine.GetInvariantExpression(System.String,Microsoft.PowerFx.Types.RecordType)
❌CP0002 M:Microsoft.PowerFx.Engine.Parse(System.String,Microsoft.PowerFx.Features,Microsoft.PowerFx.ParserOptions,System.Globalization.CultureInfo)
❌CP0002 M:Microsoft.PowerFx.Engine.Parse(System.String,Microsoft.PowerFx.ParserOptions,System.Globalization.CultureInfo)
❌CP0002 M:Microsoft.PowerFx.Engine.Tokenize(System.String)
❌CP0002 M:Microsoft.PowerFx.ParserOptions.#ctor
❌CP0002 M:Microsoft.PowerFx.PowerFxConfig.#ctor(System.Globalization.CultureInfo,Microsoft.PowerFx.Features)
❌CP0002 M:Microsoft.PowerFx.PowerFxConfig.#ctor(System.Globalization.CultureInfo)
❌CP0002 M:Microsoft.PowerFx.PowerFxConfig.get_CultureInfo

CarlosFigueiraMSFT
CarlosFigueiraMSFT approved these changes Apr 4, 2023
View reviewed changes
Copy link
Contributor

@CarlosFigueiraMSFT CarlosFigueiraMSFT left a comment

Choose a reason for hiding this comment

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

:shipit:

@LucGenetier
Copy link
Contributor Author

Public API changes:
❌CP0002 M:Microsoft.PowerFx.Engine.CreateFieldRenamer(Microsoft.PowerFx.Types.RecordType,Microsoft.PowerFx.Core.Utils.DPath,Microsoft.PowerFx.Core.Utils.DName)
❌CP0002 M:Microsoft.PowerFx.Engine.GetDisplayExpression(System.String,Microsoft.PowerFx.ReadOnlySymbolTable)
❌CP0002 M:Microsoft.PowerFx.Engine.GetDisplayExpression(System.String,Microsoft.PowerFx.Types.RecordType)
❌CP0002 M:Microsoft.PowerFx.Engine.GetInvariantExpression(System.String,Microsoft.PowerFx.Types.RecordType)
❌CP0002 M:Microsoft.PowerFx.Engine.Parse(System.String,Microsoft.PowerFx.Features,Microsoft.PowerFx.ParserOptions,System.Globalization.CultureInfo)
❌CP0002 M:Microsoft.PowerFx.Engine.Parse(System.String,Microsoft.PowerFx.ParserOptions,System.Globalization.CultureInfo)
❌CP0002 M:Microsoft.PowerFx.Engine.Tokenize(System.String)
❌CP0002 M:Microsoft.PowerFx.ParserOptions.#ctor
❌CP0002 M:Microsoft.PowerFx.PowerFxConfig.#ctor(System.Globalization.CultureInfo,Microsoft.PowerFx.Features)
❌CP0002 M:Microsoft.PowerFx.PowerFxConfig.#ctor(System.Globalization.CultureInfo)
❌CP0002 M:Microsoft.PowerFx.PowerFxConfig.get_CultureInfo

@LucGenetier LucGenetier merged commit b79d6c9 into main Apr 4, 2023
@MikeStall MikeStall mentioned this pull request Apr 5, 2023
Closed
@LucGenetier LucGenetier deleted the Config_CultureInfo2 branch April 12, 2023 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CarlosFigueiraMSFT CarlosFigueiraMSFT CarlosFigueiraMSFT approved these changes

@MikeStall MikeStall MikeStall left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LucGenetier @MikeStall @CarlosFigueiraMSFT