JacquesCarette / Drasil

Generate all the things (focusing on research software)
https://jacquescarette.github.io/Drasil
BSD 2-Clause "Simplified" License
141 stars 26 forks source link

Generate new NameChunk using `the`? #1047

Closed halonazhao closed 3 years ago

halonazhao commented 5 years ago

There is an issue about generating new NameChunk which may be improper. An example of using: https://github.com/JacquesCarette/Drasil/blob/404219e0a2d24e7ce7dadce3b3106935fc1eb53f/code/drasil-docLang/Drasil/Sections/Stakeholders.hs#L23

Source code: https://github.com/JacquesCarette/Drasil/blob/404219e0a2d24e7ce7dadce3b3106935fc1eb53f/code/drasil-data/Data/Drasil/Phrase.hs#L106-L112

JacquesCarette commented 5 years ago

Could you please add all uses, of both the and theCustom to this issue?

halonazhao commented 5 years ago

yes, but I may have to do it later because I am going to then class. I think I can finish before 2:00pm today.

halonazhao commented 5 years ago

https://github.com/JacquesCarette/Drasil/blob/404219e0a2d24e7ce7dadce3b3106935fc1eb53f/code/drasil-docLang/Drasil/Sections/Stakeholders.hs#L23 https://github.com/JacquesCarette/Drasil/blob/404219e0a2d24e7ce7dadce3b3106935fc1eb53f/code/drasil-docLang/Drasil/Sections/Stakeholders.hs#L29 https://github.com/JacquesCarette/Drasil/blob/404219e0a2d24e7ce7dadce3b3106935fc1eb53f/code/drasil-docLang/Drasil/Sections/Stakeholders.hs#L38 https://github.com/JacquesCarette/Drasil/blob/404219e0a2d24e7ce7dadce3b3106935fc1eb53f/code/drasil-docLang/Drasil/DocLang/SRS.hs#L56-L57 https://github.com/JacquesCarette/Drasil/blob/404219e0a2d24e7ce7dadce3b3106935fc1eb53f/code/drasil-data/Data/Drasil/Concepts/Documentation.hs#L202-L203 These are the current ones I can find.

halonazhao commented 5 years ago

But maybe the problem is bigger than just the constructor.

There are others who auto generate new NamedChunk like: https://github.com/JacquesCarette/Drasil/blob/404219e0a2d24e7ce7dadce3b3106935fc1eb53f/code/drasil-data/Data/Drasil/Phrase.hs#L106-L142

It is fine if we just use these constructor to have new NamedChunks with defining them first. But if directly use them in the sentence to printout, that would be hard to collect them to our database. So they should be used carefully.

JacquesCarette commented 5 years ago

I agree that we should be careful about our uses of the and theCustom. The uses you have found above are not crucial though. I think we can use Sentence-level combinators rather than NamedChunk-level with no loss in meaning or expressiveness.

halonazhao commented 5 years ago

Yes, I think so. I will change the and theCustom to generate Sentence for now. If others need the change too, we can do those later.

JacquesCarette commented 5 years ago

You might need them to generate a NounPhrase instead. Then a variant of titleize that works on NounPhrase instead of NamedChunk would mean that the overall changes to the code and examples would be small.

halonazhao commented 5 years ago

When I tried to implement to generate a NP, I found out that the phrase cannot print out NP because NP doesn't have NamedIdea Class. I guess there only left two ways of doing this.

  1. Still, make the generates as NamedChunk, but it should be defined first and then use. But this way as we said doesn't apply the concept.

  2. Totally remove the constructor, maybe make the to be at sentence level to take sentence and append the before it. But this way, those fancy control of printing (like titleize, at_start) would be lost functionally.

I think about this conflicts. My thought is that there is no reason NP cannot be printed out. So not only NamedChunk should be able to be printed out, so does NP, which leads me to think that... Should be the NP including NamedChunk instead of NamedChunk including NounPhrase. Then the NP could have some meaningless NP like the or of and so on and then other some components with NamedIdea like NamedChunks.

The way we have now makes sense too though. I don't know.

JacquesCarette commented 5 years ago

You will need to adjust the types to do this. The current combinators make too many assumptions. Below is some untested code, but which should be roughly right:

the :: (NounPhrase t) => t -> NP
 the t = nounPhrase'' (S "the" +:+ phraseNP t) (S "the" +:+ pluralNP t) CapFirst CapWords)

I think this will end up being the easiest way forward. The above should be defined in NounPhrase.hs.

halonazhao commented 5 years ago

My question is actually the next step. Once a namedChunk is changed to NP by the, NP cannot be printed out. phrase can only print chunks which have NamedIdea, but NP does not have anymore. So the the constructor will "eat" the namedIdea of a namedChunk.

Be more clear, maybe we can use S to construct NP to become a sentence. But the initial namedChunk's UID will be lost.

JacquesCarette commented 5 years ago

which is why you'll need to use phraseNP instead of phrase in the surrounding context that calls the. The examples you showed above seemed to be amenable to such a change.

halonazhao commented 5 years ago

The current capitalization doesn't work well on the new the constructor. The reason is as below: https://github.com/JacquesCarette/Drasil/blob/46f9d16782da3f3b52618624842ad5b070518e30/code/drasil-data/Data/Drasil/Phrase.hs#L109-L110 https://github.com/JacquesCarette/Drasil/blob/46f9d16782da3f3b52618624842ad5b070518e30/code/drasil-lang/Language/Drasil/NounPhrase.hs#L122-L123 https://github.com/JacquesCarette/Drasil/blob/46f9d16782da3f3b52618624842ad5b070518e30/code/drasil-lang/Language/Drasil/NounPhrase.hs#L181 https://github.com/JacquesCarette/Drasil/blob/46f9d16782da3f3b52618624842ad5b070518e30/code/drasil-lang/Language/Drasil/NounPhrase.hs#L47 https://github.com/JacquesCarette/Drasil/blob/46f9d16782da3f3b52618624842ad5b070518e30/code/drasil-lang/Language/Drasil/NounPhrase.hs#L197-L207 https://github.com/JacquesCarette/Drasil/blob/46f9d16782da3f3b52618624842ad5b070518e30/code/drasil-lang/Language/Drasil/Development/Sentence.hs#L35-L37 The cap function is the where the problem is. Since the phrase is changed to use Ch constructor, but in cap function, only S can be matched. But it is hard to solve from here because Ch takes UID so the original chunk won't be rendered until the print layer. So my thought is have different the to directly use titleizeNP to conctrol in there. Like https://github.com/JacquesCarette/Drasil/blob/46f9d16782da3f3b52618624842ad5b070518e30/code/drasil-data/Data/Drasil/Phrase.hs#L106-L107 this would work, but not sure whether you think it's ok. @JacquesCarette

JacquesCarette commented 5 years ago

You are correct that the problem is in cap. See how the last part of cap is a "catch all" case? If it was not there, we would have been alerted, when adding the new Ch that something was odd here.

I think the correct fix is to add logic to cap which takes a Ch and changes the style. That will also delay things to printing, but in a better way.

Ant13731 commented 3 years ago

I think the problems detailed in this issue (for the most part) were resolved from #2635, so it can be closed. The only small bits left to fix are now detailed in #2650 and #2651.