search

freeciv / freeciv-web

Freeciv-web is an Open Source strategy game implemented in HTML5 and WebGL, which can be played online against other players, or in single player mode against AI opponents.
Other
1.99k stars 329 forks source link

Issues reported by lgtm.com code review #177

Open andreasrosdal opened 6 years ago

andreasrosdal commented 6 years ago

https://lgtm.com/projects/g/freeciv/freeciv-web/

https://lgtm.com/projects/g/freeciv/freeciv-web/alerts/?mode=list

Some of these can be good tasks for new developers interested in small, fun programming tasks.

lonemadmax commented 6 years ago

Don't be overwhelmed by the quantity. Just take a stab at an area you feel confident with or of which you want to know more. No need to solve them all at a time!

devansh289 commented 6 years ago

Hey, I'm a beginner and would love to work on these.

ErakordnePraht commented 5 years ago

Can anyone tell me what i did was correct? I'm not very familiar with python and i don't know how to test my integration.

zekoz commented 5 years ago

@truberton @devansh289 @mchenryc Do you prefer IRC or discord for talking to the other developers?

mchenryc commented 5 years ago

@truberton you should submit a PR for your change, which will instigate the discussion of it.

If you're not familiar with the fork-pull model used by freeciv-web, github has lots of documentation, start with: https://help.github.com/articles/creating-a-pull-request-from-a-fork/ . Submit a PR, and we can discuss forks/branches, submitting PRs and referencing issues there.

fxedel commented 5 years ago

To mention @mchenryc here:

PR should generally have a purpose other than"fixing code" - the thing with changing lines just for the sake of style or syntax changes is that it hides the previous "logic" change. In other words, the why a line of codes existence, can usually be determined by the comments in the previous commit for that line and those around it - git blame is used to see why a change was made, understand the context, and determine if, and how it can/should be modified. To change code simply for the sake of whitespace, or re-arranging lines is frowned upon, because it unnecessarily hides/buries history.

That said, personally I love deleting dead code and encourage it when possible! As long as it's actually dead code, deletion doesn't change the history of the working code around it.

The exception to "don't change for the sake of" is when explicit cleanup tasks are warranted/desired. There is an open issue to clean up code smells with many commits already accepted towards that end, and others actively being worked on. Such changes are welcome - reference the issue in your PR, and keep it on task.

Maybe one could add this to the initial comment.

GaeaKat commented 5 years ago

https://lgtm.com/projects/g/freeciv/freeciv-web/alerts/?mode=list is forbidden when you try to look Going from https://lgtm.com/projects/g/freeciv/freeciv-web/ seems to work better