search

jonascarpay / apecs

a fast, extensible, type driven Haskell ECS framework for games
392 stars 43 forks source link

Replace mkName with quotes wherever appropriate #117

Closed glocq closed 1 year ago

glocq commented 1 year ago

This addresses issue #116. Given my lack of experience with Template Haskell, I recommend carefully reviewing my changes before merging. I only checked that my code compiles with this version of the library (~well, actually, I didn't even get my code to compile yet; but the errors are now unrelated to the TH-generated code.~ Update: My code now compiles fine!).

jonascarpay commented 1 year ago

Great, thanks for opening a PR. What sort of issues did you run into compiling your code? I haven't written any TH in ages, so I'm not confident either saying this will break anything, but CI's green so things appear to be fine. As far as I'm concerned, we can try releasing and then reverting if it ends up causing issues. Thoughts, @dpwiz?

glocq commented 1 year ago

What sort of issues did you run into compiling your code?

Just what I'm referring to in issue #116: the compiler was complaining that it didn't find some functions/type constructors (like Has, asks), even though I didn't directly made use of them as is in my code, and that's because I either hadn't imported them, or imported them qualified. I can provide error messages if needed.

(or maybe you mean issues in my own code? I'm just learning to use apecs-stm so I had an issue related to a misunderstanding about how the library works, but nothing relevant here.)

CI's green so things appear to be fine.

I don't know what checks CI does exactly, but I'd just like to point out that (as I realized with this issue) your library can compile fine and yet feature a TH function that generates invalid Haskell code...

dpwiz commented 1 year ago

This looks correct. Using compiler-provided facilities for names is preferable to relying on happy coincidences (:

I, too, was pleasantly surprised recently that GHC can work with imported names automatically.

dpwiz commented 1 year ago

It works on older GHCs, so I'm going to merge this right away. Thanks @glocq !