corvus-dotnet / corvus-dotnet/Corvus.Tenancy

TenantExtensions pollutes String and Guid with extension methods

Open
#81 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C#
Stars
6
Forks
1
PR merge metrics
No merged PRs in 30d

Description

Defining extension methods for basic types such as `string` is controversial. Some people love it. @idg10 happens to hate it, and although he respects other developers' right to disagree, he disapproves strongly of having such extension methods forced on you. If you're going to do it, put it in its own namespace so I don't suddenly get all these unwanted extension methods as a side effect of writing a `using` directive for some other reason.

The `TestExtensions` class is in the `Corvus.Tenancy` namespace, and so you get all of its extensions as a result of using tenancy whether you want them or not. This includes a couple of extension methods for `string` whose names give you no clue that they are specialized for processing strings containing tenant IDs.

I think it's debatable whether these should even be public. Is it intentional that tenant IDs will be parsed by clients? We've not documented the structure, so I would prefer it if these things remained opaque. But even if we do think it's a good idea for `Corvus.Tenancy.Abstractions` to provide a random selection of methods for parsing tenancy identifier strings, I don't want those showing up with generic names like `GetParentId` just because I happened to write `using Corvus.Tenancy`.

My preferences, in descending order would be:

1. Rename `TenantExtensions` to `TenantIdentifier`, make it `internal`, and change all methods to non-extension methods
1. Add a `TenantIdentifier` class, move all the extension methods on `String` and `Guid` in there, changing them to non-extension methods, let the extension methods on `ITenant` remain as extension methods, and move `TenantExtensions` into a `Corvus.Tenancy.Extensions` namespace.
1. Add a `TenantIdentifier` class, move all the non-extension methods out of `TenantExtensions` into there, and move `TenantExtensions` into a `Corvus.Tenancy.Extensions` namespace.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.