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

Replace schema traits with interface constants #485

Merged
merged 53 commits into from
Sep 9, 2022
Merged

Conversation

caendesilva
Copy link
Member

@caendesilva caendesilva commented Sep 9, 2022

The reason the schema traits were introduced was to clearly separate and define the recognized front matter options, however having it in a trait doesn't really make too much sense as they are not intended for direct reuse, but instead as a a static reference, and since they shouldn't change, why not have them as constants, and why not have those constants in an interface? It's something I only recently realized works, so let's try it out.

The schema traits were an experiment to begin with, and removing them will reduce a lot of complexity and extra code that is not needed. I'm also removing the inline documentation, as the schemas should be stable and not change a lot, and if a property needs to be explained to be understandable, it might need renaming anyways.

A large problem with using the traits for schemas is that they define the internal data type, which may not correspond with the front matter version. For example, an author is always internally parsed into an Author model, but to define one you either need to use a string or array in the front matter.

@codecov
Copy link

codecov bot commented Sep 9, 2022

Codecov Report

Merging #485 (f00be08) into master (84d140c) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #485      +/-   ##
============================================
- Coverage     99.95%   99.95%   -0.01%     
+ Complexity      934      927       -7     
============================================
  Files           108      107       -1     
  Lines          2299     2280      -19     
============================================
- Hits           2298     2279      -19     
  Misses            1        1              
Impacted Files Coverage Δ
packages/framework/src/Concerns/AbstractPage.php 100.00% <ø> (ø)
...s/framework/src/Models/Pages/DocumentationPage.php 100.00% <ø> (ø)
...ckages/framework/src/Models/Pages/MarkdownPost.php 100.00% <ø> (ø)
...s/framework/src/Concerns/ConstructsPageSchemas.php 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@caendesilva caendesilva marked this pull request as ready for review September 9, 2022 19:13
@caendesilva caendesilva merged commit c6d817e into master Sep 9, 2022
@caendesilva caendesilva deleted the rewrite-schemas branch September 9, 2022 19:26
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