-
Notifications
You must be signed in to change notification settings - Fork 1
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
OSOE-514: Deployment action with GitHub Actions and better configuration #25
Conversation
@@ -0,0 +1,28 @@ | |||
name: Deploy Media Theme |
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'm not sure about this and the workflow being here vs LGHA, but it feels wrong to add such project-specific GHA code to the general LGHA.
…eme folder in GHA
This reverts commit 52ce287.
Lombiq.Hosting.MediaTheme.Bridge/Services/FileVersionProviderDecorator.cs
Outdated
Show resolved
Hide resolved
dynamic featureStep = new JObject(); | ||
featureStep.name = "Feature"; | ||
featureStep.enable = new JArray("Lombiq.Hosting.MediaTheme.Bridge", "Lombiq.Hosting.MediaTheme"); | ||
recipeSteps.Add(featureStep); |
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.
dynamic 🤮🤮🤮
Could you replace this and the others below with stuff like this?
var featureStep = JObject.FromObject(new
{
name = "Feature",
enable = new[] { "Lombiq.Hosting.MediaTheme.Bridge", "Lombiq.Hosting.MediaTheme" },
});
Or better yet, the recipeSteps
could be a List<object>
and then it'll get converted when it's passed to its holder object in CreateRecipeAndWriteIt()
. Personally I'd replace the whole thing with POCOs, but at least we should get rid of that cursed keyword.
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.
Be not afraid!
I'm not sure if it was worth it, but I got rid of dynamic
. I don't see why we'd use List<object>
as the parameter in CreateRecipeAndWriteIt()
, since we're building a piece of JSON, so using JArray
and friends is adequate.
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'm not sure if it was worth it
It's always worth it, dynamic
is from the devil.
I don't see why we'd use List as the parameter
All you do with
recipeSteps
is pass it intoCreateRecipeAndWriteIt()
where it becomes a property of an object that gets converted via itsJObject.FromObject()
anyway. So it's not actually necessary to convert the individual items one by one.
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'd like to believe JObject.FromObject()
doesn't need to work on converting a JArray
to a JArray
:). List<object>
is really a catch-all, as mentioned I see the semantics of JArray
more appropriate, and in practice there's no difference.
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.
My point is about code simplicity, not performance. For example I figure it's better to do
JObject.From( { array = new List<object> { item1, item2, item3 } } )
than
JObject.From( { array = new JArray() { JObject.From(item1), JObject.From(item2), JObject.From(item3) } } )
I don't think littering the code with JSON-specific abstraction is helpful when you are just building an object.
Co-authored-by: Dávid El-Saig <[email protected]>
OSOE-514
Fixes #21.
Fixes #22.
Fixes #26.
Fixes #27.