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

Replaces NotImplementedException with NotSupportedException #296

Merged
merged 17 commits into from
Mar 7, 2022
Merged

Replaces NotImplementedException with NotSupportedException #296

merged 17 commits into from
Mar 7, 2022

Conversation

bijington
Copy link
Contributor

Description of Change

Replaces all usages of NotImplementedException with NotSupportedException.

Linked Issues

PR Checklist

Additional information

@bijington
Copy link
Contributor Author

Some comments online suggest we may be able to prevent the usage of NotImplementedException (https://stackoverflow.com/a/69689012/32348) however they do point to CA1031 which looks aimed at preventing its use inside a catch rather than specifically preventing the throwing of it.

@bijington
Copy link
Contributor Author

The introduction of BaseOneWayConverter may or may not complicate the support of nulls inside converters as mentioned here: #164. Note while this discussion is closed the functionality was added to the Xamarin Community Toolkit.

@brminnick brminnick mentioned this pull request Mar 2, 2022
Merged
@brminnick
Copy link
Collaborator

brminnick commented Mar 2, 2022 •
edited

Hey Shaun! I just had a moment of inspiration about adding null support for BaseOneWayConverter<TFrom, TTo>, and I figured I'd open up a PR to help explain what I'm thinking.

Check it out and let me know what you think!

#310

@brminnick
Add Null Support for BaseOneWayConverter<TFrom, TTo> (#310)
314a65d 
* Handle Nullable

* Update ImageResourceConverter

* Update IsStringNotNullOrEmptyConverter

* Update EqualConverter

* Update Remaining Converters

* Update BaseConverterOneWay.shared.cs

* Update BaseConverterOneWay.shared.cs

* Update IsStringNotNullOrEmptyConverter.shared.cs

* Update Defunct XML Comment

* Fix Null Dereference

* Remove double cast

* Convert `!=` to `is not`

* Remove unnecessary `throw new NullReferenceException`

* Add `ArgumentException` to null check

* Update BaseConverterOneWay.shared.cs
@brminnick
Merge branch 'main' into feature/not-supported-exception
da2a5b9
@brminnick
Ensure null Argument throws ArgumentNullException
b3b21a7
@brminnick
Use correct TargetType on Unit Tests
9f7d917
@brminnick
Add XAML Confirmation
36dc4f1
@brminnick
Update IsString* Converters + Unit Tests
859ed71
@brminnick brminnick changed the title Feature/not supported exception Replaces NotImplementedException with NotSupportedException Mar 3, 2022
@brminnick
Copy link
Collaborator

brminnick commented Mar 3, 2022 •
edited

Some comments online suggest we may be able to prevent the usage of NotImplementedException (https://stackoverflow.com/a/69689012/32348) however they do point to CA1031 which looks aimed at preventing its use inside a catch rather than specifically preventing the throwing of it.

Dang - yea, I can't seem to find anything either that would tell the compiler to throw an error when it detects NotImplementedException.

Since this PR is growing a bit in scope, I recommend doing the following, then let's merge this PR:

  • Update the Contribution Guidelines to recommend against NotImplementedException
  • Open a new Discussion that focuses on preventing NotImplementedException via editorconfig
    • Maybe someday in the future we, or someone smarter than us, can figure out how to do it

@brminnick
Copy link
Collaborator

  • Open a new Discussion that focuses on preventing NotImplementedException via editorconfig
  • Maybe someday in the future we, or someone smarter than us, can figure out how to do it

Discussion opened: #313

@brminnick
Copy link
Collaborator

Hey @bijington! I've updated Contributing.md to include NotImplementedException.

I'm going to move this PR out of Draft because I think it's ready to be merged now that we've moved the NotImplementedException discussion to #313.

@brminnick brminnick marked this pull request as ready for review March 4, 2022 19:30
brminnick
brminnick previously approved these changes Mar 4, 2022
View reviewed changes
bijington
bijington commented Mar 5, 2022
View reviewed changes
Copy link
Contributor Author

@bijington bijington left a comment

Choose a reason for hiding this comment

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

@brminnick thanks for the updates to this PR :)

CONTRIBUTING.md Outdated Show resolved Hide resolved
brminnick
brminnick approved these changes Mar 5, 2022
View reviewed changes
Copy link
Collaborator

@brminnick brminnick left a comment

Choose a reason for hiding this comment

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

Thanks Shaun! I'm good for you to merge this 🙌

@bijington bijington merged commit 8a8aec3 into main Mar 7, 2022
@delete-merged-branch delete-merged-branch bot deleted the feature/not-supported-exception branch March 7, 2022 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brminnick brminnick brminnick approved these changes

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

Successfully merging this pull request may close these issues.

[Proposal] Replace all NotImplementedException throws with NotSupportedException
2 participants
@bijington @brminnick