Skip to content

Switch ch Sentence constructor from using HasUID to IsChunk.#4725

Merged
JacquesCarette merged 6 commits intomainfrom
switchChIsChunkNoUID
Feb 3, 2026
Merged

Switch ch Sentence constructor from using HasUID to IsChunk.#4725
JacquesCarette merged 6 commits intomainfrom
switchChIsChunkNoUID

Conversation

@balacij
Copy link
Collaborator

@balacij balacij commented Jan 31, 2026

Builds on #4723 and #4724

This will simplify work necessary for re-writing #4716.

As a result of this, it was sensible to also replace HasUID usage in
NamedIdea to IsChunk as well (this results in a smaller diff, and is
sensible; UIDs should be best kept as 'secret' as possible, even
though this doesn't really do that).

This will simplify work necessary for re-writing #4716.

As a result of this, it was sensible to also replace `HasUID` usage in
`NamedIdea` to `IsChunk` as well (this results in a smaller diff, and is
sensible; `UID`s should be best kept as 'secret' as possible, even
though this doesn't really do that).
@balacij balacij force-pushed the switchChIsChunkNoUID branch from 06d420e to 216fcd5 Compare February 1, 2026 02:05
@balacij
Copy link
Collaborator Author

balacij commented Feb 2, 2026

Merged in main to resolve the conflict. Ready for review!

Copy link
Collaborator

@samm82 samm82 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

definedIn''' :: (HasSymbol q, HasUID q, Referable r, HasShortName r) => q -> r -> Sentence
-- | Takes a 'Symbol' and its 'Reference' (does not append a period at the
-- end!). Outputs as "@symbol@ is defined in @source@".
definedIn''' :: (Quantity q, Referable r, HasShortName r) => q -> r -> Sentence
Copy link
Owner

Choose a reason for hiding this comment

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

I don't mind not using HasUID directly in most code, but replacing HasSymbol with Quantity, even though it appears that things still work, seem like too radical a narrowing. Doesn't Quantity imply quite a few more things, none of which are actually needed?

Why not just change HasUID to IsChunk and leave the HasSymbol alone?

Copy link
Collaborator Author

@balacij balacij Feb 2, 2026

Choose a reason for hiding this comment

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

Looks like I was rushing and didn't think this one through fully -- it can have a broader scope. d620ad4 fixes this.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, but you only did it in this one spot, while the same narrow scope occurred in most of the other files you edited.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, thank you. 0a46379 resolves the remaining ones now.

@JacquesCarette JacquesCarette merged commit cb5018b into main Feb 3, 2026
5 checks passed
@JacquesCarette JacquesCarette deleted the switchChIsChunkNoUID branch February 3, 2026 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants