few comments/suggestions about CONTRIBUTING_A_NEW_QUEST.md

I was using https://github.com/matkoniecz/Zazolc/blob/so-you-want-to-make-a-new-quest/CONTRIBUTING_A_NEW_QUEST.md while trying out writing my first quest, here are some experiences/suggestions I've noted (while it's still fresh in my mind). Feel free to ask for clarifications. Some of it is probably just my confusion, but some might be actionable. Also note that I did not use whole of the documentation (most notably I've not used anything related to Android Studio not have I yet tried advanced parts)

There are few broken links, here is what I caught:
- Read what governs their priority but do not worry too much, it can be tweaked later. - both links are broken ( https://github.com/matkoniecz/Zazolc/blob/so-you-want-to-make-a-new-quest/app/src/main/java/de/westnordost/streetcomplete/quests/QuestModule.kt & https://github.com/streetcomplete/StreetComplete/blob/master/app/src/main/java/de/westnordost/streetcomplete/quests/QuestModule.kt#L138-L162)
- See also other optional properties (https://github.com/matkoniecz/Zazolc/blob/so-you-want-to-make-a-new-quest/de/westnordost/streetcomplete/data/osm/osmquests/OsmElementQuestType.kt)
when linking files and referring to specific line numbers, it would be better to reference specific commit version of that file (even if older/possibly obsolete) then the master. For example, in implement a fully custom tag parsing, still combined with filter syntax links to https://github.com/streetcomplete/StreetComplete/blob/master/app/src/main/java/de/westnordost/streetcomplete/quests/opening_hours/AddOpeningHours.kt#L137-L151 which seems to be wrong line numbers currently.
good idea to limit quests to an inclusive list of certain countries would be useful to link to examples with include as well as exclude countries.
broken markdown formatting around:
- nametag is not used, and
- which will be replaced by text provided by
I was little confused by %1$s / (%2$s) and %s in getTitle and getTitleArgs section. Is %s just a shorthand for %1$s if there is only one string to replace, or what? (I'm used to C printf(3) so that was strange / confusing. Copy/paste from other quest worked just fine, though)
Filtering in the element selectionensures that every object will have either name or brand tag., might also mention other way used in quests, how to detect missing tag and use another string value (like I copied from AirConditioning quest and it seems to work )
should code authors be mentioned somewhere? or just authors of images (which is documented in docs)?
maybe document how to do resurvey quest? I've tried copy/pasting something for my smoking quest, but I'm not sure if that is all that is needed (or whether it is completely wrong). How do the values change when one selects shorter/longer resurvey periods in preferences?
although docs suggest to start with way_lit quest for simple yes/no quest, I initially found that more complicated (several files - what is supposed to go in each one? what is sealed interface WayLitOrIsStepsAnswer, object IsActuallyStepsAnswer : WayLitOrIsStepsAnswer etc) so I've used one-file simpler AirConditioning quest as starting point. Don't know if there are better ones. Once I've had basic quest working I've borrowed some code from way_lit and wheelchair_access quests (which wasn't trouble free as I had problems compiling, but eventually after several tries figured it out). So maybe good idea would be to show one simplest quest (like Defibrilator as mentioned elsewhere in docs, or Airconditioning) and then one a little more (but not too much) complex (like way_lit) and highlight differences between them
when talking about icons More info about icon handling will be given later it might be good to make it a link to an anchor, as there seems to be section named Adding quest icon already written (I didn't have to use it, though, as my icon was already existing, so can't say how good it is)
test section mentions See "logcat" (bottom left area of the screen) to see stacktrace or logging messages. - it would be good to link to example how to make your code log something into logcat when debugging.
many quests seem to use getTitleArgs to return tags["name"] ?: tags["brand"] - I assume ?: means use first value, unless it is undefined, then use seconds one? Might want to explain that syntax. (or maybe should that name_or_brand() be consolidated as seems to be often used?)

@mnalis I fixed obvious issues (see https://github.com/matkoniecz/Zazolc/commits/so-you-want-to-make-a-new-quest ) and will work on trickier ones.

Thanks for a great review!

when linking files and referring to specific line numbers, it would be better to reference specific commit version of that file (even if older/possibly obsolete) then the master. For example, in implement a fully custom tag parsing, still combined with filter syntax links to https://github.com/streetcomplete/StreetComplete/blob/master/app/src/main/java/de/westnordost/streetcomplete/quests/opening_hours/AddOpeningHours.kt#L137-L151 which seems to be wrong line numbers currently.

I hope that recent changes (some triggered by writing this texts, some by linting) will stabilize. I will review links and set reminder to do it again in few months.

Filtering in the element selectionensures that every object will have either name or brand tag., might also mention other way used in quests, how to detect missing tag and use another string value (like I copied from AirConditioning quest and it seems to work )

I will think here.

should code authors be mentioned somewhere? or just authors of images (which is documented in docs)?

This is a bit in limbo, see https://github.com/streetcomplete/StreetComplete/pull/2815 Will update once situation is clear.

although docs suggest to start with way_lit quest for simple yes/no quest, I initially found that more complicated (several files - what is supposed to go in each one? what is sealed interface WayLitOrIsStepsAnswer, object IsActuallyStepsAnswer : WayLitOrIsStepsAnswer etc) so I've used one-file simpler AirConditioning quest as starting point. Don't know if there are better ones. Once I've had basic quest working I've borrowed some code from way_lit and wheelchair_access quests (which wasn't trouble free as I had problems compiling, but eventually after several tries figured it out). So maybe good idea would be to show one simplest quest (like Defibrilator as mentioned elsewhere in docs, or Airconditioning) and then one a little more (but not too much) complex (like way_lit) and highlight differences between them

Yeaaaah, good point. Sorry for a trap.

EDIT: Changed

test section mentions See "logcat" (bottom left area of the screen) to see stacktrace or logging messages. - it would be good to link to example how to make your code log something into logcat when debugging.

That is worth mentioning but that part will have link at most to keep it relatively simple.

many quests seem to use getTitleArgs to return tags["name"] ?: tags["brand"] - I assume ?: means use first value, unless it is undefined, then use seconds one? Might want to explain that syntax. (or maybe should that name_or_brand() be consolidated as seems to be often used?)

https://github.com/matkoniecz/Zazolc/blob/so-you-want-to-make-a-new-quest/CONTRIBUTING_A_NEW_QUEST.md#gettitle-and-gettitleargs is trying to explain that, is more info needed?

https://github.com/matkoniecz/Zazolc/blob/so-you-want-to-make-a-new-quest/CONTRIBUTING_A_NEW_QUEST.md#gettitle-and-gettitleargs is trying to explain that, is more info needed?

Well, I think so..

Docs certainly try to explain what is being done, but confusing part for me (as non-kotlin programmer) was how it is being done. Without knowing the difference between %s and %1$s (much clearer after update; but I'm still not sure if there is any difference for single-string replacements, or if I could use either one? Just curious myself, it is not important for writing quest as it is spelled out when to use which), and not knowing what ?: and arrayOfNotNull() actually do, it is not trivial to connect the dots between "what" and "how".

On the other hand, I quite understand that this should not be "introduction to kotlin for dummies" document :smile:

Here are few comments on what was confusing to me; I'm not sure if all of it is actionable or not, though, but this is a diary of how I got confused:

explanation quotes getTitle() and getTitleArgs() but not hasFeatureName(tags) which is used by them
hasFeatureName(tags) is somewhat confusing name - especially when its code for it is not shown (like it isn't shown in the docs) - I would assume it checks if any of the tags are present in current object. Of course when I eventually looked up the code, it is not at all what it does: tags is actually a list of all tags in current object (node/way/relation), and the function checks it against hard-coded values "name" and "brand". IMHO it would have been much clearer if that check tags.containsKey("name") || tags.containsKey("brand") was directly inline in getTitle(), instead of being separated in separate function, but perhaps it is done because in some other parts of code it is much more complex function? Or even better if one could say something like tags.match("name|brand") or something like that in regex format like elementFilter uses.
docs say feature type (such as "greengrocer", while the variable is named featureName - those should both use the variable name to be clear (or user is unsure if feature name / type are the same thing)
while I know it should not be all-encompasing guide to kotlin, perhaps those few nontrivial (for non-kotlin programmers) lines should be briefly explained in https://codegolf.stackexchange.com fashion, e.g.:
- name = tags["name"] ?: tags["brand"] sets the name to value of tag name, or if it is not defined, to value of tag brand. name will be undefined if none of those tags exist.
- arrayOfNotNull(name, featureName.value) - featureName.value is always defined (e.g. "greengrocer"). If name is also defined, this will return array of two values. If name is undefined, arrayOfNotNull() will return array of just one element (featureName.value)

much clearer after update; but I'm still not sure if there is any difference for single-string replacements, or if I could use either one?

I am pretty sure that yes, for single replacement both are equivalent (but has not tested it). I even suspect that for multipreplacement %s would be treated as %1s$ (I also has not tested it - but this would be silly to use).

Sadly, official docs are quite poor as usual.

hasFeatureName(tags) is somewhat confusing name

I agree, though I have no idea for a better one (if you have an idea for a clearly better one it can be changed - like https://github.com/streetcomplete/StreetComplete/commit/551e272019e475e9bde6e0498b93d2e153e0b05d which was done as part of writing this docs)

IMHO it would have been much clearer if that check tags.containsKey("name") || tags.containsKey("brand") was directly inline in getTitle(), instead of being separated in separate function, but perhaps it is done because in some other parts of code it is much more complex function?

It is done this way to ensure that tags considered as giving names are the same in various parts of the code.

Otherwise SC would crash at runtime in some cases. Though maybe having instead tagsGivingName or something would be better? (It would require changing every single quest and way of doing this so it should be noticeably better)

docs say feature type (such as "greengrocer", while the variable is named featureName - those should both use the variable name to be clear (or user is unsure if feature name / type are the same thing)

hmmm

EDIT: Unified

while I know it should not be all-encompasing guide to kotlin, perhaps those few nontrivial (for non-kotlin programmers) lines should be briefly explained in https://codegolf.stackexchange.com/ fashion, e.g.:

Kotlin docs are actually quite good so maybe linking some of less usual and often useful features.

EDIT: Added

as I started to be lost I extracted remaining work to #5, #6, #7, #8

Feel free to comment there or open new issues here or comment on the PR in StreetComplete repo

Closing as it appears that all actionable by me parts are done or extracted. Sorry if I missed something, feel free to respond in this issue (especially in cases where I asked for a better ideas) or create a new issue.

matkoniecz / Zazolc

few comments/suggestions about CONTRIBUTING_A_NEW_QUEST.md #4