dennisdoomen / dennisdoomen/reflectify
[Feature]: Document indexer exclusion, caching behaviour and internal visibility in the README
- Dominant language
- C#
- Stars
- 80
- Forks
- 6
- Avg merge
- 1d 12m
- Merged PRs (30d)
- 11
Description
### Background and motivation
The README explains the API well but leaves out three behaviours that will surprise people. All three are things you can only discover by reading the source, which defeats the purpose for a package designed to be dropped into someone else's library.
**1. Indexers are silently excluded from `GetProperties`.**
`OrderedPropertyCollection.Add` skips them outright:
```csharp
if (property.IsIndexer())
{
// We explicitly skip indexers
}
```
This is a deliberate and probably correct decision, but it is invisible. The README documents `GetProperties` and, separately, `FindIndexers`, without ever connecting the two. A reader would reasonably assume `GetProperties(MemberKind.Public)` returns every public property, indexers included. Anyone building a member walker on top of this will quietly lose data and not know why.
**2. Everything is cached forever in a static dictionary.**
`ReflectorCache` is a `static readonly ConcurrentDictionary` keyed on `(Type, MemberKind)` that is never cleared. Great for performance, and there is a `PerformanceSpecs` file protecting it. But the consequences are not documented anywhere: the cache pins `Type` objects and therefore assemblies, and it grows without bound for dynamically generated types. Consumers making a decision about embedding this in their own library deserve to know. (Tracked separately as a potential code change; this issue is about documenting the current behaviour regardless of what is decided there.)
**3. All types are emitted as `internal`.**
Every type in `Reflectify.cs` is `internal`, which is exactly right for a content-only package, since it prevents two consumers from colliding. But the README never says so. The consequence a reader needs to know is that Reflectify types cannot appear in their own public API surface: you cannot expose a `MemberKind` parameter from a public method, and you cannot return `Reflector`. That is a design constraint on anyone building a library on top of it, and it is better learned from the README than from a compiler error.
A fourth, smaller one: the README says the package requires C# 12, and there is a closed issue about `LangVersion`. Worth confirming the stated requirement matches what the props file enforces, so the two do not drift.
### Alternative Concerns
- Add a short "Things to know" or "Behaviour and limitations" section to the README covering all four points. Low effort, high value, no code changes.
- Put the indexer note in the XML documentation on `GetProperties` as well, since that is where someone will actually be looking when they hit it. The doc comment currently says "Gets the public, internal, explicitly implemented and/or default properties of a type hierarchy" with no mention of the exclusion.
- Alternatively, reconsider the behaviour itself and add a `MemberKind.Indexers` flag so callers can opt in. That is an API change and belongs in its own issue, but if it were done, the documentation problem largely goes away.
### Are you willing help with a pull-request?
No
Contributor guide
Research direction
Start with the README and compare its existing API descriptions with OrderedPropertyCollection.Add, ReflectorCache, Reflectify.cs, PerformanceSpecs, the GetProperties XML documentation, and the props file. Add a concise section covering indexer exclusion, permanent caching, internal visibility, and the enforced C# version; update GetProperties documentation if appropriate, and confirm all statements match the source.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- documentation
- Issue type
- Documentation
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 76/100