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.

Sign up for GitHub

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

Jump to bottom

Database File Limit #7047

Open
wants to merge 21 commits into
base: dev/feature
Choose a base branch
from
Open

Database File Limit #7047

wants to merge 21 commits into from

Conversation

TheAbsolutionism
Copy link

@TheAbsolutionism TheAbsolutionism commented Sep 6, 2024 •
edited
Loading

Description

This PR aims to restrict the usage of a file to only one database. Precautionary measure

  • Now also fixes the databaseName field and adds a databaseType field

Target Minecraft Versions: any
Requirements: none
Related Issues: #6858

@TheAbsolutionism
Starter Commit
e931e1c
@TheAbsolutionism
Fixing
d9c733a 
Fix, the databaseName being set as the type instead of the name
Added, databaseType field
@TheAbsolutionism
Copy link
Author

Please note, to optimally fix the databaseName and the databaseType field added through this PR, by simply setting within the constructor, will be a "breaking change" - LimeGlass

TheLimeGlass
TheLimeGlass requested changes Sep 7, 2024
View reviewed changes
Copy link
Collaborator

@TheLimeGlass TheLimeGlass left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looking good.

Dev note: The breaking change is because the field behavior is being changed from type to configuration name, thus the field should be private to avoid anyone extending VariableStorage to be using old behavior.
The current VariableStorage API is very recent, and this change is minimal.

src/main/java/ch/njol/skript/variables/VariablesStorage.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/variables/VariablesStorage.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/variables/VariablesStorage.java Outdated Show resolved Hide resolved
@TheLimeGlass TheLimeGlass added enhancement Feature request, an issue about something that could be improved, or a PR improving something. breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) labels Sep 7, 2024
@TheAbsolutionism
Requested Changes
e67b6d5 
Added, Javadoc note for getUserConfigurationName()
Changed, database(Name|Type) fields to private
Changed, all usages of 'databaseName' to 'getUserConfigurationName()'
Changed, 'registeredFiles' to Set
Changed, 'name' var
@TheAbsolutionism
Copy link
Author

Question, should we also restrict the usage of same database names?

@TheAbsolutionism
Fix Load
5e8788f 
Forgot to change the initial constructor call back to original
@TheAbsolutionism
Merge branch 'dev/feature' into dev/database-file-limit
2d95bf7
@Efnilite Efnilite added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheLimeGlass TheLimeGlass TheLimeGlass approved these changes

@sovdeeth sovdeeth sovdeeth approved these changes

Assignees
No one assigned
Labels
breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) enhancement Feature request, an issue about something that could be improved, or a PR improving something. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TheAbsolutionism @sovdeeth @TheLimeGlass @Efnilite