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

Added item data, Added handlers to fetch item data and format it, Added starting gear #72

Merged
merged 11 commits into from Apr 30, 2023

Conversation

BrunoCDev
Copy link
Contributor

@BrunoCDev BrunoCDev commented Apr 27, 2023

  • Added item data in json format (folder data/items...)
  • Added some handlers/parsers to format this data into something we can actually consume (We can call generateItem() in handlers/item.py to create any item that we have data for).
  • Starting gear for each class is being generated using this and applied to the character in lobby.
  • Added enums for all items for easier development.
  • Modified starting gear to include gold for testing purposes.

@FilippoLeone
Copy link
Contributor

great work!

@BrunoCDev BrunoCDev marked this pull request as ready for review April 27, 2023 20:35
Copy link
Owner

@Snaacky Snaacky left a comment

Choose a reason for hiding this comment

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

#69 was merged first.

Copy link
Owner

@Snaacky Snaacky left a comment

Choose a reason for hiding this comment

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

Requesting changes. Keep in mind that the places that I left comments are not the only places that these same commented issues occur and that all of them need to be changed.



def create_items_per_class(char_class):
if CharacterClass.BARBARIAN == char_class:
Copy link
Owner

Choose a reason for hiding this comment

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

Would this not be simpler to be refactored as a match-case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

json_data = {}


def loadJsonData():
Copy link
Owner

Choose a reason for hiding this comment

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

Non-pythonic variable naming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

dndserver/handlers/item.py Show resolved Hide resolved
for itemType in ItemType:
type = str(itemType.value)
path = os.path.join(current_dir, "..", "data", "items", type)
path = os.path.abspath(path)
Copy link
Owner

Choose a reason for hiding this comment

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

Recasting variables is generally bad practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

type = str(itemType.value)
path = os.path.join(current_dir, "..", "data", "items", type)
path = os.path.abspath(path)
files = [file for file in os.listdir(path) if file.endswith(".json")]
Copy link
Owner

Choose a reason for hiding this comment

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

Would glob not be simpler here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't get it to work using glob, haven't used it before so not sure what I'm doing wrong. But if I use glob it includes the full path to the property key I'm trying to set. With the current implementation it sets it with the item name correctly (see screenshot).

Feel free to change this implementation if you can get it to work with glob...
image



# Gets the relevant data depending on the item name and type
def getContent(name, type, rarity):
Copy link
Owner

Choose a reason for hiding this comment

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

Same variable issue as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

content = {}
dynamic_file_name = ""
typeValue = str(type.value)
file_name_no_rarity = name + ".json"
Copy link
Owner

Choose a reason for hiding this comment

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

It would be more consistent to use f-strings instead of string concats.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

file_name_no_rarity = name + ".json"

if rarity == Rarity.NONE.value:
if file_name_no_rarity in json_data.get(typeValue, {}):
Copy link
Owner

Choose a reason for hiding this comment

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

Could you explain the point of the {} fallback instead of a usual None fallback? Is there a reason we're providing an explicit fallback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, no need, changed it to None now



# Function to be called in order to create an item
def generateItem(name, type, rarity, itemCount):
Copy link
Owner

Choose a reason for hiding this comment

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

Function name and variable name for itemCount

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

party = get_party_by_account_id(int(req.returnAccountId)) # todo: we're storing the first player as a transport and the next as a user object
party = get_party_by_account_id(
int(req.returnAccountId)
) # todo: we're storing the first player as a transport and the next as a user object
Copy link
Owner

Choose a reason for hiding this comment

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

I'm pretty sure this is no longer true and this comment can probably be removed but needs verification?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't change any of this, only change is due to formatting

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

4 participants