demo: Switch Sentence's Ch constructor's UID-based chunk references towards UIDRef-based ones.#4829
demo: Switch Sentence's Ch constructor's UID-based chunk references towards UIDRef-based ones.#4829
Sentence's Ch constructor's UID-based chunk references towards UIDRef-based ones.#4829Conversation
references towards `UIDRef`-based ones. This will remove one of the major uses of `termResolve'` (the infamous issue mentioned in #4671). However, this work needs to be broken up and better understood.
|
|
||
| -- | Constructs a 'CodeDefinition' where the underlying 'CodeChunk' is for a variable. | ||
| qtov :: CanGenCode e => QDefinition e -> CodeDefinition | ||
| qtov :: (Typeable e, CanGenCode e) => QDefinition e -> CodeDefinition |
There was a problem hiding this comment.
These Typeable constraints are a bit ugly to plaster everywhere. I'm not immediately sure how to deal with them, or if I'm just missing something obvious.
| -- | Ch looks up the term for a given 'UID' and displays the term with a given 'SentenceStyle' and 'CapitalizationRule'. | ||
| -- This allows Sentences to hold plural forms of 'NamedIdea's. | ||
| Ch :: SentenceStyle -> TermCapitalization -> UID -> Sentence | ||
| Ch :: (TypeableChunk t, Idea t) => SentenceStyle -> TermCapitalization -> UIDRef t -> Sentence |
There was a problem hiding this comment.
Again, Typeable here is ugly. Need to figure out how to deal with this nicely.
SentenceStyle can take on 3 values: Plural, Term, and Short. Plural corresponds to plural-full. We don't support plural-short. This needs to be split into two separate options.
Now, it's because this has the option to output short-form nouns that we put the constraint Idea on the chunk (Idea = NamedIdea + getA, where getA is an accessor for the short-form of a chunk's noun phrase). (explaining this is odd, we need to fix the descriptions). As a result, a lot of functions that previously formed Sentences using this constructor that only required NamedIdea constrained needed to be weakened to Idea.
There was a problem hiding this comment.
This one is more than ugly - if it is needed, it points to a bad design. Maybe in UIDRef t?
| -- | Look up the plural form of a term given a chunk database and a 'UID' associated with the term. | ||
| lookupP' :: (TypeableChunk t, Idea t) => PrintingInformation -> UIDRef t -> TermCapitalization -> Sentence | ||
| lookupP' sm ur pCap = resolveCapP pCap $ c ^. term | ||
| where c = unhideOrErr ur (sm ^. sysdb) |
There was a problem hiding this comment.
On the bright side of this PR, we get to use unhideOrErr (which looks for a single chunk UID of a specific type) rather than termResolve' which tries to cast a chunk to
| instance HasUID (ConstraintSet e) where uid = con . uid | ||
| -- | Finds the term ('NP') of the 'ConstraintSet'. | ||
| instance NamedIdea (ConstraintSet e) where term = con . term | ||
| instance Typeable e => NamedIdea (ConstraintSet e) where term = con . term |
There was a problem hiding this comment.
Again, a wart. What's odd about this is that all types are supposed to have Typeable predefined for them by GHC. But GHC yells at us about unresolved constraints if these aren't explicitly here as well.
Also, HLS warns that this is a redundant constraint! However, GHC does not!
|
The CI failed because a |
JacquesCarette
left a comment
JacquesCarette
left a comment
There was a problem hiding this comment.
I'm guessing the problem is deep in the database code.
Rather than Typeable down there, I would be tempted to use either Proxy or an explicit kind, or something like it. We want to not forget the type. Typeable lets us forget and then recover later. That's too 'dynamic'.
| -- | A NamedIdea is a 'term' that we've identified (has a 'UID') as being worthy | ||
| -- of naming. | ||
| class IsChunk c => NamedIdea c where | ||
| class (Typeable c, IsChunk c) => NamedIdea c where |
There was a problem hiding this comment.
This is the oddest Typeable, which is definitely going to propagate downstream. Why is this one needed?
| -- | Ch looks up the term for a given 'UID' and displays the term with a given 'SentenceStyle' and 'CapitalizationRule'. | ||
| -- This allows Sentences to hold plural forms of 'NamedIdea's. | ||
| Ch :: SentenceStyle -> TermCapitalization -> UID -> Sentence | ||
| Ch :: (TypeableChunk t, Idea t) => SentenceStyle -> TermCapitalization -> UIDRef t -> Sentence |
There was a problem hiding this comment.
This one is more than ugly - if it is needed, it points to a bad design. Maybe in UIDRef t?
|
As I suspected, this is all due to using I'm thinking maybe |
2 participants
This will remove one of the major uses of
termResolve'(the infamous issue mentioned in #4671).However, this work needs to be broken up and better understood. Why are all of these changes necessary? I will attach a review.
(Yes, this fails the CI, I'm aware, that's not the point of this PR.)