Closed samm82 closed 5 years ago
Great issue. Thank you @samm82.
Should we keep the "ssp_" prefixes? Other than that (and a fixme
variable in Unitals that is never used), SSP looks good to me. @smiths
I like the idea of removing the ssp_
prefix, since it is redundant. However, there might be some consequences from the change. I'd like to hear the opinion of @bmaclach, since he has worked extensively with the ssp example.
Without the "ssp" prefix some of the variable names will conflict with others from Concepts.Documentation, though this isn't a big deal because that module is imported qualified. So I don't foresee any real problems with removing "ssp", but I will point out that the pattern of prefixing those variable names with the example name is repeated in all of the examples (i.e. there is a game_refby
in GamePhysics, a glassBR_refby
in GlassBR, etc.). So if we change them in SSP, I think the others should be changed too.
With regards to the fixme
variable - good catch! In fact, fsi
, fisi
, wla
, and smsi
are not used either. I missed that block of variables during my SSP cleanup. They can all be removed.
I'll remove the prefixes in all examples then, and remove the unused variables. 👌
@bmaclach I started removing them, and in SSP, I got a naming conflict with srs
, sec
, and section
. Should I rename all but these three (ie. leave them as ssp_srs
, etc.), or should I rename none of them, so that the variable names are consistent?
Ah, I was wrong. Documentation is not imported qualified. If there are going to naming conflicts, my opinion is that it's not worth the effort to remove the prefixes. Besides, the prefixes do help with searching the code base (if we removed the prefixes and then wanted to search the code base for (what used to be) ssp_srs
, we would match with all instances of srs
which would exist in all examples). I just thought of that now. So my vote is to rename none of them.
Since the helper strings aren't fully part of this issue, and all section names look good, I'm closing this issue.
I agree with not removing the prefixes right now, although I'm not sure I completely agree with the searching for ssp_srs
justification. If the prefixes were gone, searching for srs would find the files in the SSP folder, and in the other examples. However, anybody doing the search would likely know to ignore the srs names in the other, non-SSP, folders. The location of the file gives us the scope of the name.
I've talked about a similar topic with @JacquesCarette in the past and he has pointed out that explicit, hard-coded prefixes, are an old idea from CS. I think the prefixes will eventually be removed, but for now there isn't a compelling reason to make a change.
We should indeed remove the prefixes, and then import various things qualified to deal with the conflicts. This is a good task to do when other items are stuck for some reason, as it can be done file-by-file.
@JacquesCarette, sounds like we are on the same page. @samm82, can you create an issue for removing the prefixes and adding the qualified imports. You can put it as a low priority issue.
The current section names give little information and are subject to change if sections are added/deleted/moved. They should be updated with more descriptive names that don't sacrifice traceability. Example: https://github.com/JacquesCarette/Drasil/blob/628b54d0e17d0b818be0c36480416e5bf37a2139/code/Example/Drasil/GlassBR/Body.hs#L334-L344
Although we don't know what section 5.1 is, we know that these tables belong there. This traceability should be maintained.
Also - separate branches for easy merging 😄