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

Config validators #2090

Merged
merged 43 commits into from
May 21, 2024
Merged

Config validators #2090

merged 43 commits into from
May 21, 2024

Conversation

VladTheCow
Copy link
Contributor

@VladTheCow VladTheCow commented Sep 7, 2023 •
edited by NaoUnderscore
Loading

Description

This PR offers attributes for validating config values.
It supports making own validators (just make a new attribute that implements IValidator)

[LessThan(10)]
[GreaterThan(1)]
[NonNegative]
[NonPositive]
[PossibleValues<RoleTypeId>(RoleTypeId.ClassD, RoleTypeId.Scientist)]
public SomeType SomeValue { get; set; }

Changes

  • Update C# version to 11 (attribute with generic type)
  • New interface IValidator with validate method
  • Default validators: GreaterThan, LessThan, NonNegative, NotPositive, PossibleValues
  • Log how many validations are passed

Related suggestions

Tasks

  • Make sure it works

@VladTheCow VladTheCow changed the title Config validators [IN WORK] Config validators Sep 7, 2023
@VladTheCow VladTheCow changed the title [IN WORK] Config validators Config validators Sep 8, 2023
@VladTheCow VladTheCow marked this pull request as ready for review September 8, 2023 17:58
louis1706
louis1706 suggested changes Sep 8, 2023
View reviewed changes
EXILED.props Outdated Show resolved Hide resolved
LolaLollipop
LolaLollipop suggested changes Sep 8, 2023
View reviewed changes
Copy link
Contributor

@LolaLollipop LolaLollipop left a comment

Choose a reason for hiding this comment

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

should make everything a float instead of an int (cast to float), and need greaterthaneq and lessthaneq

louis1706
louis1706 suggested changes Sep 17, 2023
View reviewed changes
Exiled.Loader/ConfigManager.cs Outdated Show resolved Hide resolved
@NaoUnderscore NaoUnderscore changed the base branch from dev to apis-rework January 14, 2024 04:40
@NaoUnderscore NaoUnderscore added the enhancement New feature or request label Jan 25, 2024
@iamalexrouse
Copy link
Member

@NaoUnderscore Is this PR 'Do Not Merge'?

@VladTheCow VladTheCow marked this pull request as draft May 18, 2024 11:17
@iamalexrouse
Copy link
Member

iamalexrouse commented May 18, 2024 •
edited
Loading

After testing, here are the issues I have noted:

  • YamlDotNet 13.7.1 is required.
  • Validator throws 2 errors instead of the intentional 1 error. Fixed

More notes may be added as we discover more issues.

@iamalexrouse iamalexrouse mentioned this pull request May 18, 2024
Closed
@VladTheCow VladTheCow marked this pull request as ready for review May 18, 2024 13:17
@VladTheCow VladTheCow added tested This PR has been built and tested and does not cause any obvious errors. and removed requires-testing Things need to be verified by an Exiled Developer/Contributor labels May 18, 2024
iamalexrouse
iamalexrouse approved these changes May 18, 2024
View reviewed changes
Copy link
Member

@iamalexrouse iamalexrouse left a comment

Choose a reason for hiding this comment

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

This is ok.

@louis1706 louis1706 self-requested a review May 18, 2024 13:36
@moddedmcplayer
Copy link
Contributor

The docs are lacking.

@iamalexrouse
Copy link
Member

The docs are lacking.

We'll work on some documentation asap.

@moddedmcplayer
Copy link
Contributor

ALEXWARELLC

Check your fork's PRs.

@VladTheCow
Copy link
Contributor Author

ALEXWARELLC

Check your fork's PRs.

Take a look at my review

iamalexrouse
iamalexrouse approved these changes May 20, 2024
View reviewed changes
Copy link
Member

@iamalexrouse iamalexrouse left a comment

Choose a reason for hiding this comment

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

👍

@NaoUnderscore NaoUnderscore merged commit e79eeeb into Exiled-Team:apis-rework May 21, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LolaLollipop LolaLollipop LolaLollipop requested changes

@iamalexrouse iamalexrouse iamalexrouse approved these changes

@louis1706 louis1706 louis1706 approved these changes

Assignees

@VladTheCow VladTheCow

Labels
enhancement New feature or request help wanted Extra attention is needed P1 First Priority regarding-api An issue or PR targeting the Exiled API project tested This PR has been built and tested and does not cause any obvious errors.
Projects
EXILED Workflow
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@VladTheCow @iamalexrouse @louis1706 @moddedmcplayer @LolaLollipop @NaoUnderscore