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

improve CultureInfoScope #2375

Merged
merged 1 commit into from Apr 12, 2021

Conversation

bollhals
Copy link
Contributor

Changes the CultureInfoScope to a readonly struct to avoid an extra allocation.
Also slightly improve performance by only reading the current cultureInfo only once instead of multiple times.

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).
  • Performance improvement
  • Refactoring (so no functional change)
  • Other (docs, build config, etc)

Checklist:

  • I've added tests for my code. (most of the time mandatory)
  • I have added an entry to the changelog. (mandatory)
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@SabotageAndi SabotageAndi added this to To do in OSS Iteration 2 Mar 30, 2021
@epresi
Copy link
Contributor

epresi commented Apr 7, 2021

Hey @bollhals,

It looks nice that you moved the whole FeatureContext into the CultureInfoScope, but now the 2 ctors are not doing the same. For example the second one has an optimization to not set if it is the same. Also it is doing something with the LanguageHelper which is not in the constructor with CultureInfo parameter.

@epresi epresi moved this from To do to In progress in OSS Iteration 2 Apr 7, 2021
@bollhals
Copy link
Contributor Author

bollhals commented Apr 7, 2021

It looks nice that you moved the whole FeatureContext into the CultureInfoScope, but now the 2 ctors are not doing the same. For example the second one has an optimization to not set if it is the same. Also it is doing something with the LanguageHelper which is not in the constructor with CultureInfo parameter.

Excellent point, I actually wanted to ask a question about it but forgot to add it...

The question is, do we want to keep the CultureInfoScope(CultureInfo) or not. (It's not needed within this codebase).

Why is it currently as it is? => I left it in as a ctor for when you explicitly want to have a culture set/swapped. So that you call the one with CultureInfo if you already know what you want, otherwise you call the one with FeatureContext and it decides for you.

If you want to keep the ctor and have similar functionality applied to it as the other ctor has, I can adapt it to this as well.

@epresi
Copy link
Contributor

epresi commented Apr 7, 2021

The question is, do we want to keep the CultureInfoScope(CultureInfo) or not. (It's not needed within this codebase).

Well, if we remove it then it will no longer work if someone used it. However in the bindings they can get the FeatureContext via DI. @SabotageAndi what do you think?

@SabotageAndi
Copy link
Contributor

CultureInfoScope is public, but I don't know why. No idea why someone would use it. And as this will go into 3.8, I have no big problems with some small public API changes.
We had in the past a separate API Changes section for these things.

@bollhals
Copy link
Contributor Author

bollhals commented Apr 7, 2021

CultureInfoScope is public, but I don't know why. No idea why someone would use it. And as this will go into 3.8, I have no big problems with some small public API changes.

We had in the past a separate API Changes section for these things.

I assume it's public because it is used in a proteced virtual method of a public type. So it probably needs to be public.

@SabotageAndi
Copy link
Contributor

Good catch @bollhals.
Let's remove the ctor with CultureInfo

@bollhals
Copy link
Contributor Author

bollhals commented Apr 9, 2021

done

@epresi epresi merged commit 90e036f into SpecFlowOSS:master Apr 12, 2021
OSS Iteration 2 automation moved this from In progress to Done Apr 12, 2021
@bollhals bollhals deleted the feature/cultureInfoScope branch April 12, 2021 13:15
Code-Grump pushed a commit to Code-Grump/SpecFlow that referenced this pull request Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants