-
Notifications
You must be signed in to change notification settings - Fork 31
Conversation
This ensures that all projects within Miunie are now set to the latest .NET Core, .NET Standard, and C# Language versions. All dependencies specified in Miunie were updated to their most recent counterparts. All classes mentioned were modified to follow StyleCop standards. One readonly value was made in EmbedConstructor to simplify .WithColor() when creating new embeds. All existing summaries were moved into HelpStrings.resx. HelpService now attempts to fetch a summary for the specified ID in SummaryAttribute for a CommandInfo, and will now use ILanguageProvider for default string values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for helping out, @AbnerSquared.
However, with .NET Standard newer isn't always better. In fact, it's the other way around.
Feel free to read through this article from MSDN.
Here's a relevant quote:
In general, we recommend you to target the lowest version of .NET Standard possible.
So if you could please revert your .NET Standard upgrades. 😅 Also, feel free to discuss these changes in #miunie-dev to perhaps avoid these in the future. 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AbnerSquared, you're kinda doing waaay more than we bargained for.
It's usually a good thing, however, I feel bad, since some of the extra things you're doing were already decided against, or not planned at all. I super don't want you to waste time. It would be best if you could always open an appropriate issue for everything extra you think is needed.
We can then perhaps tell you that it won't work or was already tried before you try it out. 😅
You're doing great job and making a lot of progress, but the whole "let's add this and that" attitude is slowing the PR down. I hope you understand. 😊
Let's patch these changes up and we'll see how it looks. 😉
We really should be targeting a specific LangVersion rather than latest.
|
Got it. I'll be honest here and say that I made the version updates blindly, and by the time I noticed when submitting a pull request, I left it in if it would fit. I'll get on those reverts, and fix the string entries. 👍 Would you suggest that I explicitly specify the |
Certainly, setting it specifically to 8.0 should be fine. |
thoughts about a possible extension? this was made to handle the internal static string ValueOrDefault(this string value, string fallback = "")
=> string.IsNullOrWhiteSpace(value)
? fallback ?? string.Empty
: value; |
Another suggested idea I had was to implement |
I think these are great suggestions, @AbnerSquared. And I think it would be great to get this PR merged as soon as possible, and create new issues for the prefix for example. It can also be something for beginners if it's simple enough. 😊 |
This update should fix all issues previously mentioned and make some methods easier to handle. Changes - Created StringExtensions, which contains two methods of ValueOrDefault(this string, string) and JoinOrDefault(this IEnumerable<string>, string, string). - Created TExtensions, which contains one method of StringJoinOrDefault(this IEnumerable<T>, Func<T, string>, string, string). - Set all LangVersion properties to 8.0. - Modified HelpCommand, MiscCommands, ProfileCommand, RemoteRepositoryCommand, and TimeCommand to store their direct summaries and include examples. - Modified HelpCommandProvider to include three new methods of GetAllModuleSections(), GetSection(ModuleInfo), and GetSection(CommandInfo). - Modified HelpCommandProvider.Search to use GetSection(Command). - Modified HelpCommandProvider.GetDefault to use GetAllModuleSections(). - Modified HelpCommandProvider.GetExamples(CommandInfo) to use .StringJoinOrDefault(). - Modified HelpResult.Sections to implement IEnumerable<HelpSection> instead. - Modified HelpResult.Sections to expose the set property. - Modified Strings.resx to make HELP_SUMMARY_EMPTY and HELP_EXAMPLE_EMPTY include multiple response values. - Modified EmbedConstructor.GetHelpEmbed(HelpResult) to use MiuniePinkColor for its EmbedBuilder. - Renamed EmbedConstructor.DefaultEmbedColor to MiuniePinkColor and removed comment. - Renamed HelpService to HelpCommandProvider. - Renamed CommandService.GetHelpService(ILanguageProvider) to CommandService.GetHelpProvider(ILanguageProvider). - Reverted all .NET Standard targets back to 2.0. - Deleted HelpStrings.resx.
This update should fix all issues previously mentioned and make some methods easier to handle. Changes
|
A quick change that fixes the previous string[].Length to now use the required IEnumerable<string>.Count().
Patches
|
Another minor fix that applies implicit declaration where it was needed, alongside adding a blank line in between a variable and a for each statement.
Formatting
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple of things we could do a bit better, but it's getting there.
Great job so far! 😊 Keep at it!
I fixed all issues, but would you want to preserve the current public HelpResult Search(string input)
{
var sections = new List<HelpSection>();
foreach (CommandInfo command in GetCommands(input))
{
sections.Add(GetSection(command));
}
return new HelpResult()
{
Sections = sections
};
} public HelpResult Search(string input)
=> new HelpResult()
{
Sections = GetCommands(input).Select(x => GetSection(x))
}; |
- Created new method GetModuleCommandBlocks(ModuleInfo) in HelpCommandProvider. - Modified ProfileCommand to use "to" instead of "for" in both summaries. - Modified HelpCommandProvider.GetDefault() to use _commandService.Modules.Select() instead. - Modified HelpCommandProvider.GetSection(ModuleInfo) to use GetModuleCommandBlocks(). - Removed HelpCommandProvider.GetAllModuleHelpSections().
Changes
|
- Modified HelpCommandProvider.Search(string) to use the similar .Select() method used in GetDefault().
Changes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay! Thanks for dealing with this, @AbnerSquared. It's as tiresome for me as it must be for you. 😅
But we're making very good progress and I hope you're learning a thing or two so that it's not a complete waste of time. 😊
I think this should be the final set of changes. All of the critical things are pointed out and if you fix those, there should be nothing else stopping this PR from being merged. 🎉
This is it, @AbnerSquared, I feel it. 😅
I'll be able to make these changes in around an hour. |
- Modify Container.AddMiunieTypes(this IServiceCollection) to include CommandService - Modify method GetHelp() in HelpCommand to handle sending the message result - Modify method GetHelp(string) in HelpCommand to handle sending the message result - Renamed HelpCommandProvider to CommandHelpProvider - Rename method GetDefault() to ForAllCommands() in CommandHelpProvider - Rename method Search(string) to FromInput(string) in CommandHelpProvider - Remove method GetHelpProvider(ILanguageProvider) from CommandHandler - Remove method ShowDefaultHelpAsync(IMessageChannel) in CommandHelpProvider - Remove method ShowCommandHelpAsync(IMessageChannel, string) from CommandHelpProvider - Delete StringExtensions - Delete TExtensions
Changes
|
There is currently only one concern I have before I think I'm good, and that is if the constructor for public HelpCommand(CommandService commandService, ILanguageProvider lang)
{
_helpProvider = new CommandHelpProvider(commandService, lang);
} This is its current state, but for me personally, it doesn't look like it follows the rule set that is wanted when initializing new classes from DI, and I just wanted to make sure before leaving it as is. |
This quick formatting fix properly renames the specified variables, as brought to light by 1n5an1ty.
This push now correctly renames the private variables mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A tiny edit before testing.
I'll make the change since it's very small and will proceed to test the feature.
Tested on Awesome job, @AbnerSquared! 🎉 |
Create Help Command and Refactor (control-net#275)
Related Issue
Fixes #227
Refactors
LangVersion
aslatest
.MiunieUserTypeReader
to adhere to StyleCop standards.EmbedConstructor.FormatReputationType(ReputationType)
to use aswitch statement
instead.EmbedConstructor
to include a new variable calledDefaultEmbedColor
, which now stores the hexadecimal value of the defaultEmbed
color, which replaces all.WithColor(R, G, B)
methods used to.WithColor(DefaultEmbedColor)
.using
inDiscordMessagesAdapter
.Changes
HelpService
,HelpResult
,HelpSection
,HelpStrings.resx
,CommandInfoExtensions
,HelpCommand
, andExamplesAttribute
.Strings.resx
andPhraseKey
to includeUSER_EMBED_HELP_TITLE
,HELP_SUMMARY_TITLE
,HELP_SUMMARY_EMPTY
,HELP_EXAMPLE_TITLE
, andHELP_EXAMPLE_EMPTY
.CommandService
to include a new method calledGetHelpService(ILanguageProvider)
, which returns a newHelpService
.EmbedConstructor
to include a new method calledCreateHelpEmbed(HelpResult)
, which now creates a newEmbed
based off of theHelpResult
provided.MiscCommands
,TimeCommand
,ProfileCommand
, andRemoteRepositoryCommand
to have all of their summaries set to the specified ID, as well as their modules having an explicitly specified name.Expected Feedback