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

Code examples: "Output" for non print statements can be confusing #2156

Open
shapito27 opened this issue Apr 6, 2022 · 16 comments
Open

Code examples: "Output" for non print statements can be confusing #2156

shapito27 opened this issue Apr 6, 2022 · 16 comments
Labels
x:action/improve Improve existing functionality/content x:knowledge/elementary Little Exercism knowledge required x:size/small Small amount of work x:type/docs Work on Documentation

Comments

@shapito27
Copy link

Hi, I started to use Exercism for practice in Golang recently and I like you product. Thank you for you job 👍🏼
I have one remark.

I've noticed that https://exercism.org/tracks/go/exercises/annalyns-infiltration has nice code example with Output after fmt.Println:

var archerIsAwake = true
var prisonerIsAwake = false
var petDogIsPresent = false
fmt.Println(CanFreePrisoner(knightIsAwake, archerIsAwake, prisonerIsAwake, petDogIsPresent))
// Output: false

but if you take a look at this example https://exercism.org/tracks/go/exercises/vehicle-purchase (and most of other)

needLicense := NeedsLicense("car")
// Output: true

it also has Output but there is no function to Print variable needLicense. It can be confusing.
I would recommend to use one style in each task: use fmt.Println() before // Output: or show value of variable in other way.

@ahmedsameha1
Copy link
Contributor

Good point.

@andrerfcsantos
Copy link
Member

I agree that showing "Output" when there's not a print instruction can be confusing, as there is no output. It's probably a good idea to remove those instances.

Other ways to express the idea can be to put the value in the same line of the variable or explicitly saying what the variable is == to:

needLicense := NeedsLicense("car")   // true

or:

needLicense := NeedsLicense("car")
// true

or even:

needLicense := NeedsLicense("car")
// needLicense == true

I tend to prefer the first two forms as they are compact and descriptive, while not having the confusing "Output" bit.

Since this is an issue that can affect several exercises, my suggestion would be to use this issue as a reference to all PRs that fix this. If someone wants to fix a particular exercise where they see this, it's ok to create a PR for a specific exercise, just make sure to reference this issue for easy tracking.

@andrerfcsantos andrerfcsantos added x:action/improve Improve existing functionality/content x:knowledge/elementary Little Exercism knowledge required x:module/concept-exercise Work on Concept Exercises x:module/practice-exercise Work on Practice Exercises x:type/content Work on content (e.g. exercises, concepts) x:size/small Small amount of work labels Apr 9, 2022
@andrerfcsantos andrerfcsantos changed the title Code examples Output Code examples: Output for non print statements can be confusing Apr 10, 2022
@andrerfcsantos andrerfcsantos changed the title Code examples: Output for non print statements can be confusing Code examples: "Output" for non print statements can be confusing Apr 10, 2022
@TristanAppDev
Copy link
Contributor

I believe the good first issue label would be a good fit for this one.

@junedev
Copy link
Member

junedev commented Apr 12, 2022

I always thought the general logic of these type of comments is that they should be written in a REPL fashion. Coming from that perspective, here some comments from my side:

  • I think removing Output when there is no print statement is good.
  • I don't think we should have print statements everywhere because that clutters the examples a lot.
  • I don't think the first and second examples are good because in the REPL logic (which a lot of people are used to from other languages), those examples would mean the return value of the assignment is true which is not the case in Go.
  • The assignment in the first and second example does not add value imo.

I would recommend to use this format as it is clear enough imo and does not include any excess code that was only added for the purpose of writing the example:

NeedsLicense("car")
// true

(Same as at the bottom of vehicle purchase, just without the "Output".)

Just for completion, the correct format with print would be:

needLicense := NeedsLicense("car")
fmt.Println(needLicense)
// Output: true

@andrerfcsantos @norbs57 WDYT about my suggestion above? In case we can agree on the two desired versions, we should add those somewhere in the track documentation.

@andrerfcsantos
Copy link
Member

@junedev That makes sense. I like it as a standard going forward.

@norbs57
Copy link
Contributor

norbs57 commented Apr 13, 2022

I agree with most of the points raised.

With regards to the "results of assignments", in the recent parsing_log_files example, I used the following notation taken from the C# track:

b = re.MatchString("[a12]")       // => true
b = re.MatchString("12abc34(ef)") // => true
b = re.MatchString(" abc!")       // => true
b = re.MatchString("123 456")     // => false    

This notation is pretty concise, but I feel ambivalent about those arrows. On the one hand, it is neat to use an arrow to indicate something being "returned". On the other hand, the arrow goes in the wrong direction from an "assignment semantics" point of view, e.g. it should be b ← true.

What do others think about this style?

@norbs57
Copy link
Contributor

norbs57 commented Apr 13, 2022

Also, I saw on the C# track that some of the examples were written just as function calls, e.g. the above example would then be written as

re.MatchString("[a12]")       
// => true
re.MatchString("12abc34(ef)") 
// => true
re.MatchString(" abc!")       
// => true
re.MatchString("123 456")     
// => false

Any thoughts?

@junedev
Copy link
Member

junedev commented Apr 13, 2022

My understanding was that the arrows are used because they appear in some form in the actual REPL that comes with the language. Not sure how it is for C#. Because a REPL is not a standard thing in Go, I am not sure it makes sense to include it.

I wouldn't worry about the direction of the arrow too much because as noted in the second comment by @norbs57, often the assignment can be removed from the example.

Re with or without error, I asked in the Go channel on Slack what people prefer.

@BethanyG
Copy link
Member

This probably doesn't help, but in case it does - here are two examples of the Python REPL. The first is the standard one that ships with Python. Note that the arrows are for inputs, and the "output"/"value" or "return" doesn't have them:

2022-04-13_12-08-44

The second 2 examples are from iPython, an "enhanced" REPL that is often used in data sci. Note that here there are explicit in and out lines. However, this is a bit misleading since out is not necessarily output. Sometimes, it's the value of a variable, sometimes the result of calling a function.

2022-04-13_12-03-26

2022-04-13_12-13-23

@junedev
Copy link
Member

junedev commented Apr 13, 2022

@BethanyG I know that officially we should use the some standard REPL format for the examples. But tbh even if a standard REPL would exist for Go, I still find this

NeedsLicense("car")
// => true

more readable than this

>>> NeedsLicense("car")
true

Do you actually always use some official REPL format in Python as described here https://exercism.org/docs/building/markdown/markdown#h-code?

@junedev
Copy link
Member

junedev commented Apr 13, 2022

I realized that there is an issue with the version without the arrow. It gets confusing when there is an additional explaining comment:

NeedsLicense("car")
// true
// This is true because ...

Also people on Slack voted for the arrow so let's go with that.

@nahuakang
Copy link
Contributor

@junedev For the scope of this issue, should the assignee apply the arrowed comment to all exercises?

@junedev
Copy link
Member

junedev commented Apr 21, 2022

I would treat the issue as a general one. So in the end it would be great to have consistent examples with either the arrow or print syntax in all concept exercises and concepts. And of course updated documentation of what we decided for here.

@andrerfcsantos
Copy link
Member

To give better visibility on exactly what needs to be done and how to contribute to this, I created this new issue with the result of the discussion here.

@junedev
Copy link
Member

junedev commented Sep 17, 2022

I am re-opening this because I think the track readme was not yet updated to include the guidelines we settled on here. This is also not mentioned in the new issue linked above so let's use this one here to track the readme update.

@junedev junedev reopened this Sep 17, 2022
@junedev
Copy link
Member

junedev commented Oct 1, 2022

Here for reference the final formats we used:

Without print:

NeedsLicense("car")
// => true

With print:

needLicense := NeedsLicense("car")
fmt.Println(needLicense)
// Output: true

@junedev junedev added x:type/docs Work on Documentation and removed good first issue x:module/concept-exercise Work on Concept Exercises x:module/practice-exercise Work on Practice Exercises x:type/content Work on content (e.g. exercises, concepts) labels Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:action/improve Improve existing functionality/content x:knowledge/elementary Little Exercism knowledge required x:size/small Small amount of work x:type/docs Work on Documentation
Projects
None yet
Development

No branches or pull requests

8 participants