fixed overlapping digits #15 #18
Conversation
juego.py
Outdated
| list_x = [int(sx(100)),int(sx(200)),int(sx(300)),int(sx(400)),int(sx(500)),int(sx(600)),int(sx(700)), | ||
| int(sx(800)),int(sx(900))] | ||
| def rand_generator_x(): | ||
| count_x=0 |
There was a problem hiding this comment.
Could you space the assignment operator from the variable name and value, helps with readability.
juego.py
Outdated
| int(sx(800)),int(sx(900))] | ||
| def rand_generator_x(): | ||
| count_x=0 | ||
| while(count_x<len(list_x)): |
There was a problem hiding this comment.
This too and you don't need the outer parentheses.
juego.py
Outdated
| count_x=0 | ||
| while(count_x<len(list_x)): | ||
| rand_coord_x = random.choice(list_x) | ||
| count_x+=1 |
juego.py
Outdated
| while(count_x<len(list_x)): | ||
| rand_coord_x = random.choice(list_x) | ||
| count_x+=1 | ||
| if(rand_coord_x not in no_repeat_check_x): |
There was a problem hiding this comment.
No need for the parentheses here too.
juego.py
Outdated
| count_x+=1 | ||
| if(rand_coord_x not in no_repeat_check_x): | ||
| no_repeat_check_x.append(rand_coord_x) | ||
| return(rand_coord_x) |
juego.py
Outdated
| list_y = [int(sx(-50)),int(sx(-40)),int(sx(-30)),int(sx(-10)),int(sx(-20)),int(sx(-0)),int(sx(-60)),int(sx(-65))] | ||
| list_x = [int(sx(100)),int(sx(200)),int(sx(300)),int(sx(400)),int(sx(500)),int(sx(600)),int(sx(700)), | ||
| int(sx(800)),int(sx(900))] |
There was a problem hiding this comment.
Is there any reason why list_x has more values than list_y?
There was a problem hiding this comment.
Thanks for reviewing. There is no pressing reason as such. Since the x and y coordinates are chosen randomly and independent of each other, availing only 6 coordinates would serve the purpose. My intention is to cover the entire screen, hence the x- coordinates ranges from 100 to 900. The purpose of slightly varying the y coordinates is to prevent the overlapping, if the multiple 4 digit number appears consecutively.The difference in the number of values wouldn't be of much concern to the output. However, I will consider adding one more value to list_y which might as well increase a possibility.
juego.py
Outdated
| if(rand_coord_x not in no_repeat_check_x): | ||
| no_repeat_check_x.append(rand_coord_x) | ||
| return(rand_coord_x) | ||
| break |
There was a problem hiding this comment.
No need to break as there's a return statement.
juego.py
Outdated
| def rand_generator_y(): | ||
| count_y=0 | ||
| while(count_y<len(list_y)): | ||
| count_y+=1 | ||
| rand_coord_y = random.choice(list_y) | ||
| if(rand_coord_y not in no_repeat_check_y): | ||
| no_repeat_check_y.append(rand_coord_y) | ||
| return(rand_coord_y) | ||
| break | ||
| return int(sx(-3)) |
There was a problem hiding this comment.
Same comments as rand_generator_x above.
juego.py
Outdated
| ans_x_coord = rand_generator_x() | ||
| ans_y_coord = rand_generator_y() | ||
| self.preguntas.add(number(ans_x_coord,ans_y_coord, fuente.render(self.resultado,True, | ||
| (random.randint(0,255),random.randint(0,255),random.randint(0,255))),True)) |
There was a problem hiding this comment.
Also kindly remove the extra spaces at the end of the line.
juego.py
Outdated
| if random.randint(0,1) == 0: | ||
| wrong = str(int(self.resultado) - random.randint(1,10)) | ||
| wrong = str(int(self.resultado) - random.randint(1,10)) |
|
Oops! I noticed the label after committing the suggested changes. |
| @@ -37,21 +37,50 @@ def __init__(self, level, fuente): | |||
| self.expresion = fuente.render(self.primero + simbolo[operador] + self.segundo + " = ? ",True,(255,0,0)) | |||
| self.resultado = str(eval(self.primero+operacion[operador]+self.segundo)) | |||
| self.vida = 0 | |||
|
|
|||
There was a problem hiding this comment.
juego.py:40:1: W293 blank line contains white space
juego.py
Outdated
|
|
||
| no_repeat_check_x = [] | ||
| no_repeat_check_y = [] | ||
| list_y = [int(sx(-90)), int(sx(-80)), int(sx(-60)), int(sx(-50)), int(sx(-40)), int(sx(-30)), |
There was a problem hiding this comment.
juego.py:43:80: E501 line too long (101 > 79 characters)
juego.py
Outdated
| no_repeat_check_x = [] | ||
| no_repeat_check_y = [] | ||
| list_y = [int(sx(-90)), int(sx(-80)), int(sx(-60)), int(sx(-50)), int(sx(-40)), int(sx(-30)), | ||
| int(sx(-20)), int(sx(-10)), int(sx(-0))] |
There was a problem hiding this comment.
juego.py:44:17: E128 continuation line under-indented for visual indent
juego.py
Outdated
| no_repeat_check_y = [] | ||
| list_y = [int(sx(-90)), int(sx(-80)), int(sx(-60)), int(sx(-50)), int(sx(-40)), int(sx(-30)), | ||
| int(sx(-20)), int(sx(-10)), int(sx(-0))] | ||
| list_x = [int(sx(100)), int(sx(200)), int(sx(300)), int(sx(400)), int(sx(500)), int(sx(600)), |
There was a problem hiding this comment.
juego.py:45:80: E501 line too long (101 > 79 characters)
juego.py
Outdated
| list_y = [int(sx(-90)), int(sx(-80)), int(sx(-60)), int(sx(-50)), int(sx(-40)), int(sx(-30)), | ||
| int(sx(-20)), int(sx(-10)), int(sx(-0))] | ||
| list_x = [int(sx(100)), int(sx(200)), int(sx(300)), int(sx(400)), int(sx(500)), int(sx(600)), | ||
| int(sx(700)), int(sx(800)), |
There was a problem hiding this comment.
juego.py:46:17: E128 continuation line under-indented for visual indent
juego.py:46:44: W291 trailing whitespace
juego.py
Outdated
| if rand_coord_x not in no_repeat_check_x: | ||
| no_repeat_check_x.append(rand_coord_x) | ||
| return rand_coord_x | ||
| return int(sx(1000)) |
There was a problem hiding this comment.
juego.py:57:19: E271 multiple spaces after keyword
juego.py
Outdated
| rand_coord_y = random.choice(list_y) | ||
| if rand_coord_y not in no_repeat_check_y: | ||
| no_repeat_check_y.append(rand_coord_y) | ||
| return rand_coord_y |
There was a problem hiding this comment.
juego.py:66:40: W291 trailing whitespace
juego.py
Outdated
| no_repeat_check_y.append(rand_coord_y) | ||
| return rand_coord_y | ||
| return int(sx(-70)) | ||
|
|
There was a problem hiding this comment.
juego.py:68:1: W293 blank line contains whitespace
juego.py
Outdated
| ans_x_coord = rand_generator_x() | ||
| ans_y_coord = rand_generator_y() | ||
| self.preguntas.add(number(ans_x_coord,ans_y_coord, fuente.render(self.resultado,True, | ||
| (random.randint(0,255),random.randint(0,255),random.randint(0,255))),True)) | ||
| for i in range(0,5): |
There was a problem hiding this comment.
juego.py:72:80: E501 line too long (93 > 79 characters)
juego.py:72:88: E231 missing whitespace after ','
juego.py:73:33: E128 continuation line under-indented for visual indent
juego.py:73:50: E231 missing whitespace after ','
juego.py:73:55: E231 missing whitespace after ','
juego.py:73:72: E231 missing whitespace after ','
juego.py:73:77: E231 missing whitespace after ','
juego.py:73:80: E501 line too long (107 > 79 characters)
juego.py:73:94: E231 missing whitespace after ','
juego.py:73:101: E231 missing whitespace after ','
juego.py:74:25: E231 missing whitespace after ',' ```
juego.py
Outdated
| self.preguntas.add(number(wrong_x_coord,wrong_y_coord,image_wrong,False)) | ||
|
|
There was a problem hiding this comment.
juego.py:82:66: E231 missing whitespace after ','
juego.py:82:78: E231 missing whitespace after ','
juego.py:82:80: E501 line too long (85 > 79 characters)
juego.py:83:1: W293 blank line contains whitespace```
|
I had seen the changes were not flake8, but decided not to intervene.
Perhaps we should not be automatic about it. Current head of master
file juego.py has 328 lines, and 325 flake8 warnings. If we were to
encourage changes that comply with flake8 instead of the existing
style, we would have a file that has inconsistent style.
A file with inconsistent style is possibly harder to read than a file
with consistent style. One gets used to whatever style is visible.
Yet we don't want to have to review flake8 changes to the whole file.
For interest, `autopep8 -a -p 80 -i juego.py` brings it down to 55
warnings. Further manipulation of command line options may improve.
|
I agree, although she said this particular PR doesn't add new flake8 errors but I haven't checked for the existing errors and compared it to this one. |
|
Agreed, the current master branch has a lot of flake8 errors. |
|
I tried using autopep8 as suggested by @quozl. It reduced the warnings and errors. I looked into pep8 document to improve the rest manually but ended up with getting more errors. Can you suggest some resources that might help to improve? |
|
Not really. One method is to share the warning you are having problems resolving, or search for the warning in quotes. By far the best resource is the source code of flake8, but few people have the time to read it.
My suggestion is for us to review and accept the changes without concern over flake8, given that the existing situation is so bad. Then flake8 fixes can happen in the next pull request. Otherwise you could be stuck for ages on flake8 fixes.
Or, make sure you have not added warnings in your changes on this pull request. You can use git to rewind to before your work, save the flake8 output in a file, and compare with what you have now.
|
I think you misunderstood what @quozl said, please remove the |
|
Thanks, I'll do it. |
Sorry, I couldn't catch your words initially. I compared the warnings from previous code to my changes . There were 5 new warnings. I was able to fix those manually but I couldn't fix one warning. @JuiP kindly review :) |
|
You say you have an E501 at line 83. I'm not seeing that warning. Here's how I checked;
git fetch origin pull/18/head
git checkout 21ac517
python3 -m flake8 juego.py
I do see an E501 at line 39, but that line does not look difficult to
fix.
Are you sure you are running flake8 against your current branch head?
|
|
I think I didn't convey the issue properly. This error is from before the 21ac517 commit. Now since I tried to fix E501 in line 83 by applying line break, I got this error. Also, there are roughly 50 E501 and F405 errors in the code prior to my changes . When I try to fix E501 by adding a line break, it leads to E127 or E128 errors. I do not know how else to fix them. One option is to add flake8 file which ignores these error codes(I'm not aware of the cons). Here are those errors |
|
Okay, taking your 21ac517 the three introduced flake8 errors you refer to can be fixed with this change; diff --git a/juego.py b/juego.py
index 08d716d..a8e4954 100644
--- a/juego.py
+++ b/juego.py
@@ -76,12 +76,13 @@ class expresion:
self.preguntas = pygame.sprite.Group()
ans_x_coord = rand_generator_x()
ans_y_coord = rand_generator_y()
- self.preguntas.add(number(ans_x_coord, ans_y_coord,
- fuente.render(self.resultado, True,
- (random.randint(0, 255),
- random.randint(0, 255),
- random.randint(0, 255))),
- True))
+ self.preguntas.add(
+ number(ans_x_coord, ans_y_coord,
+ fuente.render(
+ self.resultado, True, (random.randint(0, 255),
+ random.randint(0, 255),
+ random.randint(0, 255))),
+ True))
for i in range(0, 5):
if random.randint(0, 1) == 0:
wrong = str(int(self.resultado) - random.randint(1, 10))This vertically aligns the True with the argument list in which it appears; easing comprehension for a new reader. A few lines down from here, you can see where a previous author has created local variables such as diff --git a/juego.py b/juego.py
index 08d716d..1564de0 100644
--- a/juego.py
+++ b/juego.py
@@ -76,30 +76,25 @@ class expresion:
self.preguntas = pygame.sprite.Group()
ans_x_coord = rand_generator_x()
ans_y_coord = rand_generator_y()
- self.preguntas.add(number(ans_x_coord, ans_y_coord,
- fuente.render(self.resultado, True,
- (random.randint(0, 255),
- random.randint(0, 255),
- random.randint(0, 255))),
- True))
+ self.preguntas.add(
+ number(ans_x_coord, ans_y_coord,
+ fuente.render(
+ self.resultado, True, (random.randint(0, 255),
+ random.randint(0, 255),
+ random.randint(0, 255))),
+ True))
for i in range(0, 5):
if random.randint(0, 1) == 0:
wrong = str(int(self.resultado) - random.randint(1, 10))
else:
wrong = str(int(self.resultado) + random.randint(1, 10))
- wrong_x_coord = rand_generator_x()
- wrong_y_coord = rand_generator_y()
- image_wrong = fuente.render(
- wrong, True, (random.randint(
- 0, 255), random.randint(
- 0, 255), random.randint(
- 0, 255)))
self.preguntas.add(
- number(
- wrong_x_coord,
- wrong_y_coord,
- image_wrong,
- False))
+ number(rand_generator_x(), rand_generator_y(),
+ fuente.render(
+ wrong, True, (random.randint(0, 255),
+ random.randint(0, 255),
+ random.randint(0, 255))),
+ False))
def cargar_imagen(nombre, trasnparent=False):I hope that shows how E127 and E128 after E501 can be remediated. |
|
Thanks, I was able to fix all E501 errors. Should I create a file to ignore F405 warnings? |
|
No, the F403 and F405 should be fixed. Also the F841. Once these are fixed, flake8 will be able to check much more, and we'll need a .flake8 to suppress E402, see the Chat Activity for example file and explanation. |
|
okay, I'll fix it in next PR |
Summary
Fixes #15. When the number falls in each iteration (each question), the multiple wrong options does not overlap over each other and over the right answer. Also, the right answer appears on different co-ordinates in each iteration. I have limited the number of options to six.
The activity works fine when played in easy, medium and difficult mode.
Tested on Sugar Desktop 0.117, Ubuntu 20.10.