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
(sitegen) Refactor how default config values are set #54
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
using System; | ||
using FluentAssertions; | ||
using Sitegen.Models; | ||
using Sitegen.Models.Config; | ||
using Xunit; | ||
|
||
namespace Sitegen.Tests | ||
{ | ||
public static class ReadConfig | ||
{ | ||
public class WithEmptyConfig | ||
{ | ||
[Fact] | ||
void throws_expected_exception() | ||
{ | ||
Action action = () => Program.ReadConfig("fixtures/empty_config.yaml"); | ||
|
||
action.Should() | ||
.Throw<ConfigurationException>() | ||
.WithMessage("config.yaml does not contain any settings"); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This illustrates my point in https://github.com/perlun/sitegen/pull/54/files#r670768803. I want to be able to group test methods like this ("With empty config", "With non existent config") and have some shared state between test methods for these grouped test methods. It's really useful once you've gotten used to it. |
||
|
||
public class WithNonExistentConfig | ||
{ | ||
[Fact] | ||
void throws_expected_exception() | ||
{ | ||
Action action = () => Program.ReadConfig("fixtures/non_existent_config.yaml"); | ||
|
||
action.Should() | ||
.Throw<ConfigurationException>() | ||
.WithMessage("fixtures/non_existent_config.yaml does not exist"); | ||
} | ||
} | ||
|
||
public class ConfigWithEmptyDictionaries | ||
{ | ||
private static readonly TopLevelConfig TopLevelConfig = Program.ReadConfig("fixtures/config_with_empty_dictionaries.yaml"); | ||
|
||
[Fact] | ||
void creates_a_non_null_TopLevelConfig_Config() | ||
{ | ||
Assert.NotNull(TopLevelConfig.Config); | ||
} | ||
|
||
[Fact] | ||
void creates_a_Config_with_the_expected_SourceDir() | ||
{ | ||
Assert.Equal("src", TopLevelConfig.Config.SourceDir); | ||
} | ||
|
||
[Fact] | ||
void creates_a_Config_with_the_expected_LayoutsDir() | ||
{ | ||
Assert.Equal("src/_layouts", TopLevelConfig.Config.LayoutsDir); | ||
} | ||
|
||
[Fact] | ||
void creates_a_Config_with_the_expected_OutputDir() | ||
{ | ||
Assert.Equal("out", TopLevelConfig.Config.OutputDir); | ||
} | ||
|
||
[Fact] | ||
void creates_a_Config_with_the_expected_PostDir () | ||
{ | ||
Assert.Equal("src/_posts", TopLevelConfig.Config.PostsDir); | ||
} | ||
|
||
[Fact] | ||
void creates_a_Config_with_the_expected_LineBreaks_setting () | ||
{ | ||
Assert.Equal(LineBreaks.Hard, TopLevelConfig.Config.LineBreaks); | ||
} | ||
|
||
[Fact] | ||
void creates_a_TopLevelConfig_with_a_non_null_Site() | ||
{ | ||
Assert.NotNull(TopLevelConfig.Site); | ||
} | ||
} | ||
} | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
config: {} | ||
site: {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,2 @@ | ||
# Empty config | ||
--- |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,26 @@ | ||
using System.IO; | ||
|
||
namespace Sitegen.Models.Config | ||
{ | ||
public class Config | ||
public class Config : IDeserialized | ||
{ | ||
public string SourceDir { get; set; } | ||
public string LayoutsDir { get; set; } | ||
public string OutputDir { get; set; } | ||
public string PostsDir { get; set; } | ||
public LineBreaks? LineBreaks { get; set; } | ||
public bool MultipleLanguages { get; set; } | ||
|
||
public void OnDeserialized() | ||
{ | ||
SourceDir ??= "src"; | ||
LayoutsDir ??= Path.Join(SourceDir, "_layouts"); | ||
OutputDir ??= "out"; | ||
PostsDir ??= "src/_posts"; | ||
|
||
// Enabling "soft line breaks as hard" is currently the default. Can be opted out by individual blog posts | ||
// as needed. | ||
LineBreaks ??= Models.LineBreaks.Hard; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ☝️ This is the essence of this PR. Moving this code out from the |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,11 @@ | ||
using JetBrains.Annotations; | ||
|
||
namespace Sitegen.Models.Config | ||
{ | ||
/// <summary> | ||
/// Top-level class used when deserializing `config.yaml` files. | ||
/// </summary> | ||
[UsedImplicitly] | ||
public class TopLevelConfig | ||
{ | ||
public Config Config { get; set; } | ||
public Site Site { get; set; } | ||
public Config Config { get; set; } = new(); | ||
public Site Site { get; set; } = new(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
namespace Sitegen.Models | ||
{ | ||
public interface IDeserialized | ||
{ | ||
/// <summary> | ||
/// Event which will be called after deserialization of the object (graph) is complete. | ||
/// </summary> | ||
void OnDeserialized(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
using Sitegen.Services; | ||
using Sitegen.Services.MultiLanguage; | ||
using Sitegen.Services.SingleLanguage; | ||
using Sitegen.YamlDotNet; | ||
using YamlDotNet.Core; | ||
using YamlDotNet.Serialization; | ||
using YamlDotNet.Serialization.NamingConventions; | ||
|
@@ -107,22 +108,33 @@ private Program(TopLevelConfig topLevelConfig) | |
|
||
public static TopLevelConfig ReadConfig(string path) | ||
{ | ||
if (!File.Exists(path)) | ||
{ | ||
throw new ConfigurationException($"{path} does not exist"); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (One nice side feature of unit testing is that you tend to more carefully test various error scenarios when writing tests. This tends to improve the overall error handling of your software.) |
||
var input = new StringReader(File.ReadAllText(path)); | ||
|
||
var deserializer = new DeserializerBuilder() | ||
.WithNamingConvention(UnderscoredNamingConvention.Instance) | ||
.WithSerializationEventSupport() | ||
.Build(); | ||
|
||
try | ||
{ | ||
var config = deserializer.Deserialize<TopLevelConfig>(input); | ||
|
||
// A YAML document without any content (or with content consisting solely of comments) will yield a | ||
// 'null' reference at this point. I haven't found this explicitly documented in YamlDotNet, but that's | ||
// the semantics I'm currently seeing. | ||
config ??= new TopLevelConfig(); | ||
if (config == null) | ||
{ | ||
// While this logic might seem silly and irrelevant, it does serve a purpose. Empty YAML documents | ||
// will yield a 'null' reference at this point because of how YamlDotNet works. We must have a valid | ||
// non-null object we can use, so why not just instantiate a blank object at this point? If we do, | ||
// our custom deserialization code powered by IDeserialized will not be executed. We choose the | ||
// simple approach and make this an unsupported scenario altogether; it's very unlikely to cause any | ||
// real-world problems anyway. | ||
throw new ConfigurationException("config.yaml does not contain any settings"); | ||
} | ||
|
||
ValidateConfig(config); | ||
return config; | ||
} | ||
catch (YamlException e) | ||
|
@@ -138,20 +150,6 @@ public static TopLevelConfig ReadConfig(string path) | |
} | ||
} | ||
|
||
private static void ValidateConfig(TopLevelConfig config) | ||
{ | ||
// Set default values for config settings which have not been provided | ||
config.Config ??= new Config(); | ||
config.Config.SourceDir ??= "src"; | ||
config.Config.LayoutsDir ??= Path.Join(config.Config.SourceDir, "_layouts"); | ||
config.Config.OutputDir ??= "out"; | ||
config.Config.PostsDir ??= "src/_posts"; | ||
|
||
// Enabling "soft line breaks as hard" is currently the default. Can be opted out by individual blog posts | ||
// as needed. | ||
config.Config.LineBreaks ??= LineBreaks.Hard; | ||
} | ||
|
||
private void ConvertPosts() | ||
{ | ||
// Pass 1: convert all Markdown files to .html | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
#nullable enable | ||
|
||
using System; | ||
using Sitegen.Models; | ||
using YamlDotNet.Core; | ||
using YamlDotNet.Serialization; | ||
|
||
namespace Sitegen.YamlDotNet | ||
{ | ||
/// <summary> | ||
/// <see cref="INodeDeserializer"/> which supports emitting `OnDeserialized` events. | ||
/// </summary> | ||
public class EventSupportingNodeDeserializer : INodeDeserializer | ||
{ | ||
private readonly INodeDeserializer nodeDeserializer; | ||
|
||
public EventSupportingNodeDeserializer(INodeDeserializer nodeDeserializer) | ||
{ | ||
this.nodeDeserializer = nodeDeserializer; | ||
} | ||
|
||
public bool Deserialize(IParser parser, Type expectedType, | ||
Func<IParser, Type, object?> nestedObjectDeserializer, out object? value) | ||
{ | ||
if (!nodeDeserializer.Deserialize(parser, expectedType, nestedObjectDeserializer, out value) || | ||
value == null) | ||
{ | ||
return false; | ||
} | ||
|
||
if (value is IDeserialized deserializedObject) | ||
{ | ||
deserializedObject.OnDeserialized(); | ||
} | ||
|
||
return true; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
using YamlDotNet.Serialization; | ||
using YamlDotNet.Serialization.NodeDeserializers; | ||
|
||
namespace Sitegen.YamlDotNet | ||
{ | ||
public static class YamlDotNetExtensions | ||
{ | ||
public static DeserializerBuilder WithSerializationEventSupport(this DeserializerBuilder builder) | ||
{ | ||
return builder | ||
.WithNodeDeserializer(inner => new EventSupportingNodeDeserializer(inner), | ||
s => s.InsteadOf<ObjectNodeDeserializer>()); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up converting the test project introduced in #53 into plain C# after all. It was a specific feature in C# that was lacking in F# that I felt was so important that I didn't want to be without it - inner classes.
When writing unit tests, I like to be able to group related test methods into an inner class, for testing a particular semantic detail. Not being able to do this turned out to be a showstopper to me. While I did like the F# experiment, I simply didn't want to be without this feature available in C#.
This does seem to be on the "approved backlog" for F# though, so who knows, maybe I'll end up converting the tests back to F# at some point. 😁 fsharp/fslang-suggestions#277
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I forgot one change at this point: https://github.com/perlun/sitegen/pull/56/files#r676808497