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

Fix override attribute and rollback to old behavior #976

Closed
wants to merge 19 commits into from
Closed

Fix override attribute and rollback to old behavior #976

wants to merge 19 commits into from

Conversation

SubZero0
Copy link
Member

@SubZero0 SubZero0 commented Mar 9, 2018

This will fix the override issue ( #936 ) and rollback the behavior change introduced ( #941 )

The actual issue was that the OverrideTypeReaderAttribute was actually adding the reader to the command service (so it would be registered as a global type reader and replace the current one if it existed [if I add a reader for a non-primitive type and override it somewhere, it'll still present the same issue]).

This PR will turn the previously breaking change into a simple bug fix (no behavior change).

PS: Sorry for the commits, I couldn't properly update my fork. I need to git gud with this.

SubZero0 and others added 18 commits March 4, 2018 22:35
@SubZero0
Remove IGuild.DownloadUsersAsync() from SocketGuild (#944)
b5a7fc2
@foxbot @SubZero0
Bump version to 2.0.0-beta2
3415850
@AntiTcb @SubZero0
Change GameParty size types to longs. (#955)
bbdffd1
@foxbot @SubZero0
Add callback method for when a module class has been added (#934)
9de923b 
commit 5b047bf
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Fri Feb 2 22:22:00 2018 +0100

    [feature/OnModuleAdded] Quickstart fixes (#946)

    * Quickstart: fix minor derp

    * Other overdue fixes

commit bd3e9ee
Author: Christopher F <computerizedtaco@gmail.com>
Date:   Sat Jan 27 16:51:18 2018 -0500

    Resort usings in ModuleBase

commit 8042767
Author: Christopher F <computerizedtaco@gmail.com>
Date:   Sat Jan 27 16:41:39 2018 -0500

    Clean up removed owned IServiceProvider

commit 30066cb
Author: Christopher F <computerizedtaco@gmail.com>
Date:   Sat Jan 27 16:37:22 2018 -0500

    Remove redundant try-catch around OnModuleBuilding invocation

    If this exception is going to be rethrown, there's no reason to include
    a try-catch.

commit 60c7c31
Author: Christopher F <computerizedtaco@gmail.com>
Date:   Sat Jan 27 16:36:27 2018 -0500

    Include the ModuleBuilder in OnModuleBuilding

    This allows modules hooking into OnModuleBuilding method to mutate
    theirselves at runtime.

commit b6a9ff5
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Mon Jan 22 13:17:14 2018 +0100

    #DERP

commit f623d19
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Mon Jan 22 13:15:31 2018 +0100

    Resolution for #937 because it's literally 4 lines of code

commit 8272c96
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Mon Jan 22 11:39:28 2018 +0100

    Re-adjust quickstart

commit e30b907
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Mon Jan 22 11:35:08 2018 +0100

    Undo experimental changes, request IServiceProvider instance everywhere instead

commit ad7e0a4
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Fri Jan 19 03:40:27 2018 +0100

    Fix quickstart leftover from previous draft

commit e3349ef
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Fri Jan 19 03:33:46 2018 +0100

    Doc comment on items

commit 81bd911
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Fri Jan 19 03:16:44 2018 +0100

    Add comment about the ServiceProviderFactory in the quickstart

commit 72b5e6c
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Fri Jan 19 03:10:40 2018 +0100

    Remove superfluous comments, provide simpler alternative for setting the ServiceProvider.

commit 74b17b0
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Tue Jan 16 18:06:28 2018 +0100

    Experimental change for feedback

commit 7b100e9
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Mon Jan 15 23:34:06 2018 +0100

    * Make the service provider parameters required

    * Adjust quickstart guide to reflect changes

commit 7f1b792
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Mon Jan 15 20:04:37 2018 +0100

    I..... missed one.

commit 031b289
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Mon Jan 15 20:02:20 2018 +0100

    Rename method to more intuitive 'OnModuleBuilding'

commit 9a166ef
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Mon Jan 15 19:09:10 2018 +0100

    Add callback method for when a module class has been added to the CommandService.
@AntiTcb @SubZero0
Add request info to HttpException & RateLimitedException (#957)
a3d28b6 
* Add request info to RateLimitedException

* Remove Promise from interface.

* Add Request to HttpException.
@Anu6is @SubZero0
Incorrect variable assignment (#959)
626bbb9 
The username parameter was being used to set args.AvatarUrl as opposed to the actual avatarUrl parameter provided
@awulkan @SubZero0
[docs] Change 'Echos' to 'Echoes' (#964)
7408ac8
@foxbot @SubZero0
Don't attempt to load types with generic parameters as a module
38cdc81 
This fixes an issue where custom ModuleBases that contained a generic
parameter would be loaded as a module - only to fail when trying to be
built.

Realistically, ModuleBases _should_ be abstract - but it was still a bug
that we allowed them to be included as a module.
@FiniteReality @SubZero0
Correct impl. of HasFlag and ResolveChannel (#966)
352780d 
HasFlag was checking if any of the flags were set, not the ones
specified, and ResolveChannel was still treating the ChannelPermission
enum as before it was changed to a bitflag.
@Chris-Johnston @SubZero0
Add more tests for Permissions class (#967)
5a60ed5 
* Add tests for more Permissions code coverage

* Add guild tests

* Add more in-depth covering of permissions methods

* Add tests for OverwritePermissions

* Remove unknown ItemGroup tag from csproj

* Add missing Fact attributes, separate class so that it is not dependant on main test fixture

* Separate out GuildPermissions and ChannelPermissions tests from main partial Tests class because they do not need to have access to the main test fixture
@foxbot @SubZero0
AddModuleAsync/AddModulesAsync should not require an IServiceProvider
9a6a0b3 
If one is not passed, they will default to an EmptyServiceProvider

This resolves issues where since the merging of OnModuleBuilding, users
were effectively forced to use the DI pattern, where before they were
not.

While this isn't necessarily a bad idea, we shouldn't just change this
behavior for no reason (though I assume making the IServiceProvider
argument required was a mistake)
@AntiTcb @SubZero0
Allow nested ModuleBase classes to be built when declared from non-mo…
2a32e16 
…dule classes. (#969)

* Allow modules to be built regardless of their declaring type.

* Need to exclude submodules.
@Headline @SubZero0
Add Missing Parameter For WebSocket4Net Constructor (#968)
b4e94d1 
* Add missing param to constructor

This should fix `15:02:44 Gateway     System.MissingMethodException: Method not found: 'Void WebSocket4Net.WebSocket..ctor(System.String, System.String, System.Collections.Generic.List`1<System.Collections.Generic.KeyValuePair`2<System.String,System.String>>, System.Collections.Generic.List`1<System.Collections.Generic.KeyValuePair`2<System.String,System.String>>, System.String, System.String, WebSocket4Net.WebSocketVersion, System.Net.EndPoint)'.`

* Bump WS4N to latest stable
@SubZero0
Update fork (#2)
45f9a4d 
* Remove IGuild.DownloadUsersAsync() from SocketGuild (#944)

* Bump version to 2.0.0-beta2

* Change GameParty size types to longs. (#955)

* Add callback method for when a module class has been added (#934)

commit 5b047bf
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Fri Feb 2 22:22:00 2018 +0100

    [feature/OnModuleAdded] Quickstart fixes (#946)

    * Quickstart: fix minor derp

    * Other overdue fixes

commit bd3e9ee
Author: Christopher F <computerizedtaco@gmail.com>
Date:   Sat Jan 27 16:51:18 2018 -0500

    Resort usings in ModuleBase

commit 8042767
Author: Christopher F <computerizedtaco@gmail.com>
Date:   Sat Jan 27 16:41:39 2018 -0500

    Clean up removed owned IServiceProvider

commit 30066cb
Author: Christopher F <computerizedtaco@gmail.com>
Date:   Sat Jan 27 16:37:22 2018 -0500

    Remove redundant try-catch around OnModuleBuilding invocation

    If this exception is going to be rethrown, there's no reason to include
    a try-catch.

commit 60c7c31
Author: Christopher F <computerizedtaco@gmail.com>
Date:   Sat Jan 27 16:36:27 2018 -0500

    Include the ModuleBuilder in OnModuleBuilding

    This allows modules hooking into OnModuleBuilding method to mutate
    theirselves at runtime.

commit b6a9ff5
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Mon Jan 22 13:17:14 2018 +0100

    #DERP

commit f623d19
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Mon Jan 22 13:15:31 2018 +0100

    Resolution for #937 because it's literally 4 lines of code

commit 8272c96
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Mon Jan 22 11:39:28 2018 +0100

    Re-adjust quickstart

commit e30b907
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Mon Jan 22 11:35:08 2018 +0100

    Undo experimental changes, request IServiceProvider instance everywhere instead

commit ad7e0a4
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Fri Jan 19 03:40:27 2018 +0100

    Fix quickstart leftover from previous draft

commit e3349ef
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Fri Jan 19 03:33:46 2018 +0100

    Doc comment on items

commit 81bd911
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Fri Jan 19 03:16:44 2018 +0100

    Add comment about the ServiceProviderFactory in the quickstart

commit 72b5e6c
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Fri Jan 19 03:10:40 2018 +0100

    Remove superfluous comments, provide simpler alternative for setting the ServiceProvider.

commit 74b17b0
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Tue Jan 16 18:06:28 2018 +0100

    Experimental change for feedback

commit 7b100e9
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Mon Jan 15 23:34:06 2018 +0100

    * Make the service provider parameters required

    * Adjust quickstart guide to reflect changes

commit 7f1b792
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Mon Jan 15 20:04:37 2018 +0100

    I..... missed one.

commit 031b289
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Mon Jan 15 20:02:20 2018 +0100

    Rename method to more intuitive 'OnModuleBuilding'

commit 9a166ef
Author: Joe4evr <jii.geugten@gmail.com>
Date:   Mon Jan 15 19:09:10 2018 +0100

    Add callback method for when a module class has been added to the CommandService.

* Add request info to HttpException & RateLimitedException (#957)

* Add request info to RateLimitedException

* Remove Promise from interface.

* Add Request to HttpException.

* Incorrect variable assignment (#959)

The username parameter was being used to set args.AvatarUrl as opposed to the actual avatarUrl parameter provided

* [docs] Change 'Echos' to 'Echoes' (#964)

* Don't attempt to load types with generic parameters as a module

This fixes an issue where custom ModuleBases that contained a generic
parameter would be loaded as a module - only to fail when trying to be
built.

Realistically, ModuleBases _should_ be abstract - but it was still a bug
that we allowed them to be included as a module.

* Correct impl. of HasFlag and ResolveChannel (#966)

HasFlag was checking if any of the flags were set, not the ones
specified, and ResolveChannel was still treating the ChannelPermission
enum as before it was changed to a bitflag.

* Add more tests for Permissions class (#967)

* Add tests for more Permissions code coverage

* Add guild tests

* Add more in-depth covering of permissions methods

* Add tests for OverwritePermissions

* Remove unknown ItemGroup tag from csproj

* Add missing Fact attributes, separate class so that it is not dependant on main test fixture

* Separate out GuildPermissions and ChannelPermissions tests from main partial Tests class because they do not need to have access to the main test fixture

* AddModuleAsync/AddModulesAsync should not require an IServiceProvider

If one is not passed, they will default to an EmptyServiceProvider

This resolves issues where since the merging of OnModuleBuilding, users
were effectively forced to use the DI pattern, where before they were
not.

While this isn't necessarily a bad idea, we shouldn't just change this
behavior for no reason (though I assume making the IServiceProvider
argument required was a mistake)

* Allow nested ModuleBase classes to be built when declared from non-module classes. (#969)

* Allow modules to be built regardless of their declaring type.

* Need to exclude submodules.

* Add Missing Parameter For WebSocket4Net Constructor (#968)

* Add missing param to constructor

This should fix `15:02:44 Gateway     System.MissingMethodException: Method not found: 'Void WebSocket4Net.WebSocket..ctor(System.String, System.String, System.Collections.Generic.List`1<System.Collections.Generic.KeyValuePair`2<System.String,System.String>>, System.Collections.Generic.List`1<System.Collections.Generic.KeyValuePair`2<System.String,System.String>>, System.String, System.String, WebSocket4Net.WebSocketVersion, System.Net.EndPoint)'.`

* Bump WS4N to latest stable
@SubZero0
Merge pull request #3 from RogueException/dev
b0bae92 
Update fork
@SubZero0
Merge pull request #4 from RogueException/dev
90ecb14 
Resolve #936 (#941)
@SubZero0
Fix override and rollback to old behaviour
054e148
@SubZero0
Rollback to old behavior
a1dea2e
@SubZero0
Copy link
Member Author

SubZero0 commented Mar 9, 2018 •
edited
Loading

Just forgot to mention that if this PR is merged, #975 will become obsolete.
And right now the issue with override still exists, it's just for added types (that don't have a default type reader). And this PR will really fix it now.

foxbot
foxbot requested changes Mar 11, 2018
View reviewed changes
src/Discord.Net.Commands/Builders/ModuleClassBuilder.cs Outdated
@@ -246,7 +246,7 @@ private static void BuildParameter(ParameterBuilder builder, System.Reflection.P
builder.Summary = summary.Text;
break;
case OverrideTypeReaderAttribute typeReader:
builder.TypeReader = GetTypeReader(service, paramType, typeReader.TypeReader, services);
builder.TypeReader = ReflectionUtils.CreateObject<TypeReader>(typeReader.TypeReader.GetTypeInfo(), service, EmptyServiceProvider.Instance);
Copy link
Member

Choose a reason for hiding this comment

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

I figure this should probably be using the IServiceProvider passed to BuildParameter, no?

@SubZero0
Copy link
Member Author

After looking this PR again, my premise wasn't true, since the CommandService will store more than one reader. But since the built-in was registered in another list (_defaultTypeReaders) and the override attribute add the type reader to the "user added list" (_typeReaders), it caused the unwanted override for all.

@SubZero0 SubZero0 closed this Mar 11, 2018
@Joe4evr Joe4evr mentioned this pull request Mar 17, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@foxbot foxbot foxbot requested changes

Assignees
No one assigned
Labels
priority: bug project: commands version: patch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@SubZero0 @foxbot @AntiTcb @Anu6is @awulkan @FiniteReality @Chris-Johnston @Headline