Add plugin support for other templates languages #102

sarahelsaig · 2024-12-07T03:04:02Z

I've thought this through, and I think Roslyn C# script plugins make the most sense. (also to make it work with DLLs you'd have to publish OrchardCoreContrib.PoExtractor.Abstractions to NuGet as a separate library package - probably not worth the effort) I've included a unit test and sample plugin that hopefully gets the point across without being too niche. (The important change is in the Program.ProcessPluginsAsync method, the rest is just plumbing.)

Fixes #101

hishamco · 2024-12-07T18:05:34Z

I just looked into the PR and noticed a dependency on Program.cs which is wrong for me. IMHO exposing the IProjectProcessor will make the tool extendable. I am still thinking if we need the -p or if we can make the tool open some possibilities for the others to add their own options

Let me hear your feedback on this

sarahelsaig · 2024-12-08T14:36:09Z

I just looked into the PR and noticed a dependency on Program.cs which is wrong for me.

If you mean the ScriptOptions.Default.AddReferences(typeof(Program).Assembly) here, I've only used Program to expose all the libraries to the script the tool has already loaded. As I see it, this is not a security application so we can be very permissive.
For example this way the script has access to OrchardCoreContrib.PoExtractor.DotNet.CS too, not just OrchardCoreContrib.PoExtractor.Abstractions. If someone wants to have a different take on one of the existing processors, this could be a way to have your own custom (more opinionated) solution without it affecting the main repo, while re-using the auxiliary classes that are already in those projects. I don't see the harm in it, so could you explain why it's wrong for you?

IMHO exposing the IProjectProcessor will make the tool extendable.

I think it's not possible to expose just one type, you have to expose its whole assembly. We can change the above linked code to only expose OrchardCoreContrib.PoExtractor.Abstractions instead of everything, but that feels unnecessarily restrictive.

I am still thinking if we need the -p or if we can make the tool open some possibilities for the others to add their own options.

Sorry, I don't really understand. This -p approach is the simplest way to add broad extensibility that I could think of, while being less maintenance in your code base. If you want something more elaborate without the need to use a command line switch we can discuss, but I'm not sure if it's really necessary.

…guration.

sarahelsaig · 2024-12-09T15:40:41Z

Also by importing Program's assembly the plugin developer will have access to all the of external dependencies PoExtractor has already loaded. For example if I only included the OrchardCoreContrib.PoExtractor.Abstractions, then the example plugin will fail with the following error:

error CS0234: The type or namespace name 'Json' does not exist in the namespace 'System.Text' (are you missing an assembly reference?)

So now I'd have to include #r "nuget: System.Text.Json, 9.0.0" at the start of the script, and NuGet may have to download this version if it gets out of sync from what the tool itself uses. This is annoying imo.

…tractions project.

sarahelsaig · 2024-12-14T18:39:51Z

@hishamco Have you seen my replies? This is not super urgent yet but it would be good if we could have a solution before year's end, because we might need it next month.

hishamco · 2024-12-14T18:45:00Z

I didn't look into the last comment, but the PR is opened in one tab, so I need to revise it today or tomorrow to proceed

sarahelsaig · 2024-12-30T19:02:45Z

Hi @hishamco , any news with this?

hishamco · 2024-12-31T06:00:00Z

NuGet.config

    <clear />
-    <add key="NuGet" value="https://api.nuget.org/v3/index.json" />
-    <add key="OrchardCore" value="https://nuget.cloudsmith.io/orchardcore/preview/v3/index.json" />
+    <add key="nuget.org" value="https://api.nuget.org/v3/index.json" />


The feed is already there?!!

Ooops, I didn't notice. I've reverted the name.

My point was just to remove the OrchardCore source. It's not even used and because of the central package management this kept giving warning NU1507: There are 2 package sources defined in your configuration.

If you need the Orchard Core preview feed in the future, I suggest adding the full <packageSourceMapping> configuration, as you can see in OrchardCore.Commerce.

hishamco · 2024-12-31T06:02:24Z

src/OrchardCoreContrib.PoExtractor.Abstractions/PluginHelper.cs

+        foreach (var plugin in plugins)
+        {
+            var code = await File.ReadAllTextAsync(plugin);
+            await CSharpScript.EvaluateAsync(code, options, new PluginContext(projectProcessors, projectFiles));


I'm not sure why this is in Abstractions while it relies heavily on CSharpScript

Where should it be then?

The reason is that this class only relies on IProjectProcessor from this project, which is in OrchardCoreContrib.PoExtractor.Abstractions, so I imported CSharpScript's package there as well. I think it's appropriate, because this helper is related to extensibility. We can move it into OrchardCoreContrib.PoExtractor if you prefer.

I've moved it to OrchardCoreContrib.PoExtractor.

I am not familiar with POe but anything that is related to c# plugins should be in a separate project or in the main assembly, not abstraction. No reference, no class.

hishamco · 2024-12-31T06:17:55Z

@sarahelsaig Sorry for the late, I revised the code one more time still worried about the impact of dynamic linking, so I'm thinking for two simple approaches:

Creating OrchardCoreContrib.PoExtractor.VueJs directly in the repo
Reference Lombiq.VueJs to OrchardCoreContrib.PoExtractor

While this repo is Orchard Core Contrib it SHOULD help any OC project, I might think the first option is better, but I need to hear your feedback. Also, I will think for the last time about dynamic linking and its drawbacks

sarahelsaig · 2024-12-31T09:37:01Z

Can you explain what is your worry about dynamic linking? What are the drawabacks? I don't see any impact unless you manually opt-in by using the command line switch. Otherwise I think it's safe with only very minimal overhead. I see it as a transparent improvement.

this repo is Orchard Core Contrib it SHOULD help any OC project

I fully agree with this sentiment. Note that since my original PR, I have seen other highly project-specific opportunities where this feature could be useful. In both of the cases below I could use a PoExtractor plugin to centralize other localization tech so OC can manage the actual localization and then serve it back to the associated tech. This will reduce the number of tools for the staff to learn to do the translations, and make complete translation management easier and safer.

We have a client project that uses OC as a headless server with a complementing node.js frontend. We could harvest the english l18n JSON files used by the frontend (similarly to the example plugin in this PR) and then serve back the other languages.
In the same project we also have to use a XSLT to generate some visualizations (it's a legal requirement). For now we manually maintain XML translation files for the languages we need in the MVP, but having a third localization tech in the project just for one small feature is very annoying.

I don't think we should create OrchardCoreContrib.PoExtractor.VueJs, not other OrchardCoreContrib.PoExtractor projects for the use-cases I mentioned above. This would quickly bloat your repo.

Reference Lombiq.VueJs to OrchardCoreContrib.PoExtractor

I'm not sure if I understand. If you mean using OrchardCoreContrib.PoExtractor as a library inside Lombiq.VueJs, your existing NuGet tool package doesn't make it possible. Even if you released a NuGet library package it would be pointlessly cumbersome to use in my opinion.

sarahelsaig · 2025-01-10T11:05:44Z

@hishamco Happy New Year! Have seen my last comment?

Even if you don't want to go forward with this PR I'd like to understand what's your concern about using C# scripts in this project. Maybe you are right and I should avoid using it in my own projects too.

Also just to clear things up, since you mentioned "dynamic linking" in your previous comment. The topic came up in the issue, but in this PR I did not implement dynamic linking. What we have here is just scripting. Similar to using Jint in OrchardCore, except the script language is also C#.

hishamco · 2025-01-10T11:17:20Z

Happy New Year!

Let us finalize the PR this week, it would be nice to demo it in the upcoming meeting if it's possible I need to have @sebastienros feedback

README.md

NuGet.config

hishamco · 2025-01-15T15:39:25Z

I'm planning to use CLI parsing package to make things easier, let me create a quick PR to use https://github.com/natemcmaster/CommandLineUtils the you can react to the changes, then we could merge

Hope the recording for yesterday's meeting will be published ASAP, I need to have a look before we merge this. No worry we will try to merge this PR this week

Co-authored-by: Hisham Bin Ateya <hishamco_2007@yahoo.com>

sarahelsaig · 2025-01-16T15:08:12Z

I have rewritten Program to work with your new command line argument system. I have also found and fixed a bug that we missed in the previous PR.

About the recording, maybe it will be faster if you ask for a direct link from someone who is part of the main Orchard org.

hishamco · 2025-01-16T16:11:01Z

I have rewritten Program to work with your new command line argument system. I have also found and fixed a bug that we missed in the previous PR.

I'm suggesting a new PR for it or I will do it so quick, we need to focus on what it's related to the plugin support

Thanks again

sarahelsaig · 2025-01-16T17:47:53Z

I'm suggesting a new PR for it or I will do it so quick, we need to focus on what it's related to the plugin support

Thanks again

Sure, here is the PR: #104

hishamco · 2025-01-17T20:44:03Z

I'm still waiting for Gabor to check the recording, but I will do one more review tonight

hishamco · 2025-01-20T23:14:26Z

I just saw the recording and heard @sebastienros feedback:

Shall we need to support absolute & relative URLs for plugins
@sebastienros from a security perspective do you think this is safe, especially if someone wrote a C# plugin

Last not Liquid is already supported and the tool is able to extract the strings https://github.com/OrchardCoreContrib/OrchardCoreContrib.PoExtractor/tree/main/src/OrchardCoreContrib.PoExtractor.Liquid

sarahelsaig · 2025-01-21T14:21:39Z

Shall we need to support absolute & relative URLs for plugins

Can you elaborate, what relative URL means here? I just use HttpClient without any custom configuration for loading remote plugins, so I think we always need absolute URLs.

Last not Liquid is already supported and the tool is able to extract the strings

Sorry for the confusion, that part is not related to this PR. Just a sudden though I had at the moment. I have created a separate issue to discuss: #105

src/OrchardCoreContrib.PoExtractor/PluginHelper.cs

src/OrchardCoreContrib.PoExtractor/Program.cs

test/OrchardCoreContrib.PoExtractor.Tests/PluginTests.cs

hishamco · 2025-01-21T23:26:39Z

I did a last review, please check and let merge then

Co-authored-by: Hisham Bin Ateya <hishamco_2007@yahoo.com>

hishamco · 2025-01-22T15:07:24Z

Please comment on #102 (comment) then we can merge so quick

hishamco · 2025-01-22T20:48:46Z

Thanks so much @sarahelsaig, I think the next step is to publish the abstractions project

hishamco · 2025-01-22T20:51:22Z

BTW did you start a VueJS localization extractor, or it's something private for Lombiq

sarahelsaig · 2025-01-22T21:05:31Z

I will start working on it soon (Lombiq/Orchard-Vue.js#152).
It will be open source in the https://github.com/Lombiq/Orchard-Vue.js/ repo.

sarahelsaig added 10 commits December 6, 2024 23:36

Command line refactoring.

1f1cac7

Add -p/--plugin command line option.

9eb4f21

Add Microsoft.CodeAnalysis.CSharp.Scripting package.

a8d4f00

Add CSX plugin support.

f3420df

Some documentation.

c172b43

Access bug fix.

aeaa3bd

Add sample and test.

31224e0

Fix spacing.

11b7fd5

Fix test name.

e7c2a10

Fix test for Windows.

8716a94

Fix warning NU1507: There are 2 package sources defined in your confi…

bd6cfd5

…guration.

Move ProcessPluginsAsync logic into the PluginHelper class in the Abs…

d634380

…tractions project.

hishamco reviewed Dec 31, 2024

View reviewed changes

Revert unnecessary nuget source name change.

132d4f4

sarahelsaig added 5 commits January 14, 2025 20:47

Fix spacing.

e4abfa7

prevent index out of range exception

07798dc

Minor code cleanup.

9cf8265

Add online plugin support as suggested by @sebastienros.

857c41f

Try out online referencing.

f6e0443

hishamco requested changes Jan 15, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

NuGet.config Show resolved Hide resolved

hishamco mentioned this pull request Jan 15, 2025

Use CommandLineUtils #103

Merged

sarahelsaig and others added 5 commits January 16, 2025 14:50

Update README.md

af5a94a

Co-authored-by: Hisham Bin Ateya <hishamco_2007@yahoo.com>

Merge remote-tracking branch 'origin/main' into plugin

85448b9

Post-merge rewrite.

20ecc7d

fix pluralization.

1c03367

bug fix

284e8b3

Delete src/OrchardCoreContrib.PoExtractor/GetCliOptionsResult.cs

ffac438

hishamco requested changes Jan 21, 2025

View reviewed changes

sarahelsaig and others added 5 commits January 22, 2025 15:09

Update test/OrchardCoreContrib.PoExtractor.Tests/PluginTests.cs

9ec159c

Co-authored-by: Hisham Bin Ateya <hishamco_2007@yahoo.com>

Merge remote-tracking branch 'origin/main' into plugin

dd927ad

Add missing partial to type.

fa7ec12

Update src/OrchardCoreContrib.PoExtractor/PluginHelper.cs

c5ba5db

Co-authored-by: Hisham Bin Ateya <hishamco_2007@yahoo.com>

Update naming convention in CSX and documentation.

935a85e

Simplify referencing assemblies in PluginHelper.ProcessPluginsAsync().

04f6630

sarahelsaig requested a review from hishamco January 22, 2025 15:15

hishamco approved these changes Jan 22, 2025

View reviewed changes

hishamco merged commit f799acb into OrchardCoreContrib:main Jan 22, 2025
1 check passed

Add plugin support for other templates languages #102

Add plugin support for other templates languages #102

Uh oh!

Conversation

sarahelsaig commented Dec 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hishamco commented Dec 7, 2024

Uh oh!

sarahelsaig commented Dec 8, 2024

Uh oh!

sarahelsaig commented Dec 9, 2024

Uh oh!

sarahelsaig commented Dec 14, 2024

Uh oh!

hishamco commented Dec 14, 2024

Uh oh!

sarahelsaig commented Dec 30, 2024

Uh oh!

hishamco Dec 31, 2024

Choose a reason for hiding this comment

Uh oh!

sarahelsaig Dec 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hishamco Dec 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarahelsaig Dec 31, 2024

Choose a reason for hiding this comment

Uh oh!

sarahelsaig Jan 14, 2025

Choose a reason for hiding this comment

Uh oh!

sarahelsaig Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

sebastienros Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

hishamco commented Dec 31, 2024

Uh oh!

sarahelsaig commented Dec 31, 2024

Uh oh!

sarahelsaig commented Jan 10, 2025

Uh oh!

hishamco commented Jan 10, 2025

Uh oh!

Uh oh!

Uh oh!

hishamco commented Jan 15, 2025

Uh oh!

sarahelsaig commented Jan 16, 2025

Uh oh!

hishamco commented Jan 16, 2025

Uh oh!

sarahelsaig commented Jan 16, 2025

Uh oh!

hishamco commented Jan 17, 2025

Uh oh!

hishamco commented Jan 20, 2025

Uh oh!

sarahelsaig commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hishamco commented Jan 21, 2025

Uh oh!

hishamco commented Jan 22, 2025

Uh oh!

Uh oh!

hishamco commented Jan 22, 2025

Uh oh!

hishamco commented Jan 22, 2025

Uh oh!

sarahelsaig commented Dec 7, 2024 •

edited

Loading

sarahelsaig Dec 31, 2024 •

edited

Loading

hishamco Dec 31, 2024 •

edited

Loading

sarahelsaig commented Jan 21, 2025 •

edited

Loading