Skip to content
This repository has been archived by the owner on Mar 26, 2022. It is now read-only.

Fix typos in README usage section #8

Closed
rvictorino opened this issue Apr 24, 2019 · 6 comments
Closed

Fix typos in README usage section #8

rvictorino opened this issue Apr 24, 2019 · 6 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed Typos The issue does not require any change in the codebase.

Comments

@rvictorino
Copy link

  • Trailing «r» missing for «Generator»:
SpellGenerator::Generato.new.generate #=> "Symptomatic punch"
  • Trailing «r» missing for «Generator»; missing comma between arguments if given output is what's expected:
SpellGenerator::Generato.new.self_generate('Random' 'Fire') #=> "Random Fire"

Fixing this does not require to write code (but requires to execute it to validate solution !)

@K-Sato1995 K-Sato1995 added AWESOME good first issue Good for newcomers help wanted Extra attention is needed labels Apr 24, 2019
@K-Sato1995
Copy link
Owner

This would be a great first issue as well!!
Thanks @rvictorino !!

@K-Sato1995 K-Sato1995 added Typos The issue does not require any change in the codebase. and removed AWESOME labels Apr 24, 2019
@K-Sato1995
Copy link
Owner

Closing the issue since it was fixed here.

@rvictorino
Thanks for opening the issue😇!!

@rvictorino
Copy link
Author

@K-Sato1995 both missing «r» have been fixed by the linked PR (#10 ).

However, executing :

SpellGenerator::Generator.new.self_generate('Random' 'Fire')

Outputs RandomFire, not Random Fire (notice the missing space between words).

I'm not a Ruby dev, so I may be missing some key point or syntactic sugar but adding a comma between both strings seems to help 😄

I guess, since this issues has been partially fixed, it would be logical to reopen it but maybe you would prefer that I open a new one focusing on the comma thing ?

@K-Sato1995
Copy link
Owner

K-Sato1995 commented Apr 25, 2019

@rvictorino
Hi! Thanks for this 😊!!
In ruby, you need to put , between each argument. Otherwise, Ruby's interpreter recognizes it as one string like in the example below.

# This method is expecting to get 2 arguments when it's called. 
def test(a,b)
  p "#{a} and #{b}" 
end

test('string1', 'string2') #=> "string1 and string2" # Got 2 arguments as expected.
test('string1'  'string2') #=> wrong number of arguments (given 1, expected 2) # Ruby treat it as if only one string was passed.

That said, it is wired it didn't raise any error and output RandomFire. I specified to raise an error when only one value is passed to the method here.

I just created examples/spells.rb. Can you check if you can reproduce the same case using the example??

You can run the code by following the steps below in your terminal.

cd spell_generator // move to this project's directory
ruby examples/spells.rb

@rvictorino
Copy link
Author

Hi, it's me, again !

yeah, I knew I saw something about checking number of arguments in the source code; that's what bugged me.

I can reproduce !
In the example spells file, there is a comma between arguments, so it works !

When I remove the comma, I get RandomFire (no space).

To sum things up, here are the code / outputs pairs I tested:

SpellGenerator::Generator.new.self_generate('Random', 'Fire') #=> Random Fire
SpellGenerator::Generator.new.self_generate('RandomFire') #=> RandomFire
SpellGenerator::Generator.new.self_generate('Random' 'Fire') #=> RandomFire
  • First one is what's expected
  • Second one is just one argument. Shouldn't it fire something / not work?
  • Same for the third line, except the only argument is split in two strings.

So two things:

  • As stated in this issue, README needs to be updated to reflect a working usage with two arguments: add a comma between Random and Fire.
  • There might be some unexpected behavior when this method is used with only one argument: Should I create a new issue to explain it separately?

Thanks again for those great explanations about ruby @K-Sato1995 !! 🤓 😃

@K-Sato1995
Copy link
Owner

K-Sato1995 commented Apr 26, 2019

Hi @rvictorino !
I really appreciate your thorough review of this project!!
It's always good to have a more experienced programmer like yourself looking over a project👏.

As stated in this issue, README needs to be updated to reflect a working usage with two arguments: add a comma between Random and Fire.

Yes for sure! The part about #self_generate in README.md is definitely not correct rn🙇🏼‍♀️.

There might be some unexpected behavior when this method is used with only one argument: Should I create a new issue to explain it separately?

Originally the expected behavior is to raise the original SpellGeneratorError when only one argument is passed to the method.
I would really appreciate if you open issues for those problems respectively🙇🏼‍♀️.

Thanks again!!! I truly appreciate your cooperation🥇!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers help wanted Extra attention is needed Typos The issue does not require any change in the codebase.
Projects
None yet
Development

No branches or pull requests

2 participants