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

Custom culture #296

Merged
merged 5 commits into from
Nov 15, 2023
Merged

Custom culture #296

merged 5 commits into from
Nov 15, 2023

Conversation

Keyros
Copy link
Contributor

@Keyros Keyros commented Sep 4, 2023

Fix #293 #241
Add culture support for MSDN documentation links

Included an option for users to specify their preferred culture for the MSDN documentation links

Now we can define custom culture at config.rsp file

@waf
Copy link
Owner

waf commented Sep 5, 2023

Thanks for the PR! From a quick look, it looks good. I'll review this and play around with it later this week.

@Keyros
Copy link
Contributor Author

Keyros commented Sep 14, 2023

Thanks for the PR! From a quick look, it looks good. I'll review this and play around with it later this week.

Hi, is there any news on the pull request?

Copy link
Owner

@waf waf left a comment

Choose a reason for hiding this comment

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

Just some minor feedback. Thanks for this! Happy to merge this when the PR is updated and I'll get a new release spun out for this feature.

Apologies for taking a long time for getting this reviewed. I'm in the middle of wedding planning which leaves surprisingly little time for hacking on C# :)

return culture;
}
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Are we able to use CultureInfo.GetCultureInfo(String) instead of looping through all the cultures?

If guess that if the user specifies some invalid culture, GetCultureInfo would throw -- I think it's fine to catch the exception and return some configuration error so the user knows their custom setting is not valid.

[Theory]
[InlineData("--culture", "en-gb")]
[InlineData("--culture", "en-GB")]
[InlineData("--culture", "qwe", true)]
Copy link
Owner

Choose a reason for hiding this comment

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

I think this should be an error, rather than silently ignoring the user's configuration

* CultureInfo.GetCultureInfo now throws CultureNotFoundException exception
@Keyros
Copy link
Contributor Author

Keyros commented Oct 9, 2023

Just some minor feedback. Thanks for this! Happy to merge this when the PR is updated and I'll get a new release spun out for this feature.

Apologies for taking a long time for getting this reviewed. I'm in the middle of wedding planning which leaves surprisingly little time for hacking on C# :)

@waf Congratulations!!!

Сomments were resolved, now CultureNotFoundException is thrown

@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (af2ac23) 0.0% compared to head (c4d7f14) 77.5%.
Report is 8 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##           main    #296      +/-   ##
=======================================
+ Coverage   0.0%   77.5%   +77.5%     
=======================================
  Files        83      83              
  Lines      5573    5584      +11     
  Branches    733     736       +3     
=======================================
+ Hits          0    4328    +4328     
+ Misses     5573     988    -4585     
- Partials      0     268     +268     
Files Coverage Δ
CSharpRepl.Services/Configuration.cs 81.8% <100.0%> (+81.8%) ⬆️
CSharpRepl/CommandLine.cs 94.6% <100.0%> (+94.6%) ⬆️
CSharpRepl/CSharpReplPromptCallbacks.cs 80.8% <50.0%> (+80.8%) ⬆️

... and 78 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@waf waf merged commit e4c6752 into waf:main Nov 15, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'Not found' page opens instead of documentation
2 participants