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

feat: add NearbyArtefactsList #1581

Merged
merged 5 commits into from
Oct 11, 2021
Merged

feat: add NearbyArtefactsList #1581

merged 5 commits into from
Oct 11, 2021

Conversation

razvan-pro
Copy link
Collaborator

@razvan-pro razvan-pro commented Oct 10, 2021

image


This change is Reviewable

@razvan-pro razvan-pro self-assigned this Oct 10, 2021
Copy link
Member

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @razvan-pro)


aimmo-game-worker/simulation/errors.py, line 7 at r2 (raw file):

class NoNearbyArtefactsError(Error):

What's the reason for making NoNearbyArtefactsError extend Error instead of Exception directly?


aimmo-game-worker/simulation/utils.py, line 20 at r2 (raw file):

            if len(self) == 0:
                raise NoNearbyArtefactsError(
                    "There aren't any nearby objects, you need to move closer!"

objects or artefacts?


aimmo-game-worker/simulation/world_map.py, line 8 at r2 (raw file):

from typing import Dict, List
from .pathfinding import astar
from .utils import NearbyArtefactsList

Organise imports? 😁 🙏


game_frontend/src/pyodide/webWorker.ts, line 79 at r2 (raw file):

  const matches = log.match(regexToFindNextTurnErrors)
  if (matches?.length >= 2) {
    const simpleError = matches[2].split('\n').slice(-2).join('')

I can't fully tell what this does, does it format the error message to be more readable?

Copy link
Collaborator Author

@razvan-pro razvan-pro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 of 5 files reviewed, 4 unresolved discussions (waiting on @faucomte97)


aimmo-game-worker/simulation/errors.py, line 7 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

What's the reason for making NoNearbyArtefactsError extend Error instead of Exception directly?

It's a common pattern as far as I know. I'd say for better structure and for typing as well, for example if this package is used by others, you can catch all errors thrown by it by catching its base error class.


aimmo-game-worker/simulation/utils.py, line 20 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

objects or artefacts?

"artefacts" sounds better 🙂 👍


aimmo-game-worker/simulation/world_map.py, line 8 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Organise imports? 😁 🙏

😅 😁


game_frontend/src/pyodide/webWorker.ts, line 79 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

I can't fully tell what this does, does it format the error message to be more readable?

I was a bit uncertain about this change. It actually removes the traceback and prints just the exception line. It looks like it was doing the same thing before as well - from some of my tests, it didn't change any previous functionality (also the tests that we have are passing). However, there was some traceback from the custom exception that I added, it was showing that it occurred in aimmo-game-worker/simulation/utils.py and it seemed confusing. This split/slice fixes that.

Copy link
Contributor

@dionizh dionizh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 of 5 files reviewed, 4 unresolved discussions (waiting on @faucomte97)


game_frontend/src/pyodide/webWorker.ts, line 79 at r2 (raw file):

Previously, razvan-pro (Razvan Mahu) wrote…

I was a bit uncertain about this change. It actually removes the traceback and prints just the exception line. It looks like it was doing the same thing before as well - from some of my tests, it didn't change any previous functionality (also the tests that we have are passing). However, there was some traceback from the custom exception that I added, it was showing that it occurred in aimmo-game-worker/simulation/utils.py and it seemed confusing. This split/slice fixes that.

Can we put some comments for this please?

Copy link
Member

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dionizh and @razvan-pro)


game_frontend/src/pyodide/webWorker.ts, line 79 at r2 (raw file):

Previously, dionizh (Dioni Zhong) wrote…

Can we put some comments for this please?

Yes comments :)

Copy link
Collaborator Author

@razvan-pro razvan-pro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @faucomte97)


game_frontend/src/pyodide/webWorker.ts, line 79 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Yes comments :)

yep 👍

Copy link
Member

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @razvan-pro)

Copy link
Member

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @razvan-pro)

@codecov
Copy link

codecov bot commented Oct 11, 2021

Codecov Report

Merging #1581 (ba712d1) into development (3002ffa) will increase coverage by 0.15%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1581      +/-   ##
===============================================
+ Coverage        67.55%   67.70%   +0.15%     
===============================================
  Files              161      163       +2     
  Lines             3316     3332      +16     
  Branches           234      235       +1     
===============================================
+ Hits              2240     2256      +16     
  Misses            1048     1048              
  Partials            28       28              
Impacted Files Coverage Δ
aimmo-game-worker/simulation/errors.py 100.00% <100.00%> (ø)
aimmo-game-worker/simulation/utils.py 100.00% <100.00%> (ø)
game_frontend/src/pyodide/webWorker.ts 21.62% <100.00%> (+2.17%) ⬆️

@razvan-pro razvan-pro merged commit bcab6df into development Oct 11, 2021
@razvan-pro razvan-pro deleted the nearby-artefacts-list branch October 11, 2021 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants