Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove-hardwired-symbols-and-triggerItem #294

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kayvank
Copy link
Contributor

@kayvank kayvank commented Mar 21, 2022

remove-hardwired-symbols, have not gotten to the the triggeritems yet. All build and test issues resolved.

@kayvank kayvank force-pushed the 152/remove-hardwired-symbols-and-triggerItem branch 2 times, most recently from 9343cc9 to 6fe48fa Compare March 22, 2022 04:29
@kayvank kayvank marked this pull request as ready for review March 22, 2022 04:31
@kayvank kayvank force-pushed the 152/remove-hardwired-symbols-and-triggerItem branch from 6fe48fa to a8613f5 Compare March 22, 2022 04:57
@Mikolaj
Copy link
Member

Mikolaj commented Mar 22, 2022

Cograts on making this compile. Could you try and remove most uses of toContentSymbol? In a separate commit, if you please. The goal of this exercise was to make the ContentSymbol abstract, so using toContentSymbol is a hack piercing through that abstarction, so should only be used if there's a good reason. The API to used instead of toContentSymbol is in Game.LambdaHack.Content.RuleKind (in which toContentSymbol is legit).

GameDefinition/Content/ItemKind.hs Outdated Show resolved Hide resolved
GameDefinition/game-src/Client/UI/Content/Screen.hs Outdated Show resolved Hide resolved
definition-src/Game/LambdaHack/Content/ItemKind.hs Outdated Show resolved Hide resolved
definition-src/Game/LambdaHack/Definition/Defs.hs Outdated Show resolved Hide resolved
engine-src/Game/LambdaHack/Client/UI/ActorUI.hs Outdated Show resolved Hide resolved
engine-src/Game/LambdaHack/Client/UI/ActorUI.hs Outdated Show resolved Hide resolved
engine-src/Game/LambdaHack/Client/UI/ActorUI.hs Outdated Show resolved Hide resolved
engine-src/Game/LambdaHack/Client/UI/DrawM.hs Outdated Show resolved Hide resolved
remove-hardwired-symbols-and-triggerItem
@kayvank kayvank force-pushed the 152/remove-hardwired-symbols-and-triggerItem branch from a8613f5 to 5bfaee3 Compare March 22, 2022 16:20
@kayvank kayvank requested review from Mikolaj March 23, 2022 18:12
@kayvank kayvank force-pushed the 152/remove-hardwired-symbols-and-triggerItem branch from df8be36 to 270c733 Compare March 23, 2022 23:17
@Mikolaj
Copy link
Member

Mikolaj commented Mar 24, 2022

The failed CI seem to be doctests. I guess they just need updating.

@Mikolaj
Copy link
Member

Mikolaj commented Mar 24, 2022

README tells how to run doctests. It may be easier than finding it in the CI script.

@kayvank kayvank force-pushed the 152/remove-hardwired-symbols-and-triggerItem branch from 270c733 to 4563085 Compare March 24, 2022 05:39
Issue LambdaHack#152, removing uses of toContentSymbol where possible
@kayvank kayvank force-pushed the 152/remove-hardwired-symbols-and-triggerItem branch from 4563085 to dd995ec Compare March 24, 2022 14:32
@@ -263,7 +252,7 @@ dropItems t = ([CmdItemMenu, CmdItem], t, dropCmd)

descIs :: [TriggerItem] -> Text
descIs [] = "trigger an item"
descIs (t : _) = makePhrase [tiverb t, tiobject t]
descIs _ = makePhrase ["fling", "in-range projectile"]
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather inline the whole makePhrase ["fling", "in-range projectile"] into projectI, because descIs is also used in applyIK so we can't change it freely.

issue 152, remove all hardwired item symbols as well as of TriggerItem.
This commit removes the `flingTs` function before continuing to removing
the trigger item. `flingTs`  added text to `tiverb` and `tiobject`. For now
this commit hardwired these texts in the
`Game.LambdaHack.Client.UI.Content.Input` function `descIs`
@kayvank kayvank force-pushed the 152/remove-hardwired-symbols-and-triggerItem branch from 5138a46 to 9aec886 Compare March 27, 2022 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants