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

Feature: V2 /rules endpoint #560

Merged
merged 7 commits into from
Oct 10, 2024
Merged

Conversation

calumbell
Copy link
Contributor

This PR goes part way to resolving issue #544 by introducing new models and views to API V2 for returning data equivelent to the /sections endpoint from V1 of the API.

Wait, why RuleGroup?

Following discussion on the Open5e Discord, it was decided that the names of these endpoints should be /rules for specific rules and /rulesets for set of rules. However, the endpoint /rulesets is currently being used to return a list of different 'flavours' of 5e. D&D 2014, Level Up Advanced 5e, that kind of thing. It was decided that this endpoint should be renamed to /gamesystem, so that we can use /ruleset for sets of rules.

So if that was decided, how come I used the endpoint /rulegroup? The reason is that changing the endpoint for /rulesets -> /gamesystems would require not only updating models, views, serializers, &c., but also updating a lot of the V2 data, and also updating the V1->V2 converters. In other words, it would be a monster of a PR touching many different parts of the code-base, and likely be an absolute nightmare to review.

So /rulegroup is a temporary name used to split this issue into managable chunks

@augustjohnson
Copy link
Collaborator

I have refactored the old ruleset to gamesystem. Pull in changes from staging and adjust your code to use the different ruleset wording.

Copy link
Collaborator

@augustjohnson augustjohnson left a comment

Choose a reason for hiding this comment

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

Sending this back for now, merge in staging to your dev branch and refactor rulegroup to ruleset.

You'll probably have to pipenv install -d because of the new dependencies.

@calumbell
Copy link
Contributor Author

@augustjohnson Thanks for making the change from RuleSet -> GameSystem. Requested changes to this PR have been made. RuleGroup is now RuleSet and works as intended. Merging from staging proved painless! Let me know if you'd like anything else done before approving!

Copy link
Collaborator

Choose a reason for hiding this comment

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

filename should be singular, rule.py.


class RuleSet(HasName, HasDescription, FromDocument):
"""
The RuleGroup model contains a set of Rules as part of a larger article, or
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still a reference to ruleGroup here. Edit comments.

Copy link
Collaborator

@augustjohnson augustjohnson left a comment

Choose a reason for hiding this comment

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

Just a couple of very small changes.

  • models/rules.py should be renamed rule.py
  • any (comment) references to RuleGroup should be rewritten. I saw 2.

@augustjohnson augustjohnson merged commit 5c98d41 into open5e:staging Oct 10, 2024
2 checks passed
@calumbell calumbell mentioned this pull request Oct 16, 2024
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.

2 participants