freeciv-web
freeciv-web copied to clipboard
freeciv
Issues reported by lgtm.com code review
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.
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!
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.
@truberton @devansh289 @mchenryc Do you prefer IRC or discord for talking to the other developers?
@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.
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.
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
