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

A little cleaning #191

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Conversation

olegkaspersky
Copy link

Small changes to check if you guys accept PRs from external ppl

I assume that plugin classes aren't supposed to be used anywhere outside of this module since all dependencies are private in Build.cs file
@@ -103,7 +103,7 @@ class FGitSourceControlModule : public IModuleInterface
return GitSourceControlProvider;
}

GITSOURCECONTROL_API static const TArray< FString > & GetEmptyStringArray()
static const TArray< FString > & GetEmptyStringArray()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why API removed?

Copy link
Author

@olegkaspersky olegkaspersky Nov 17, 2024

Choose a reason for hiding this comment

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

I don't know how you use it internally, but in my projects, I interact with VCS through ISourceControlModule, ISourceControlProvider and UEditorAssetLibrary, UEditorAssetSubsystem in my validators and asset action utilities

So exposing such functionality seems a bit wrong architecturally, but again I'm not aware of how you use it in your code, so feel free to revert my changes

At least exposing GITSOURCECONTROL_API static const TArray< FString > & GetEmptyStringArray() seems a bit cursed

@@ -16,7 +16,7 @@
"Modules": [
{
"Name": "GitSourceControl",
"Type": "UncookedOnly",
"Type": "Editor",
Copy link
Contributor

Choose a reason for hiding this comment

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

To my knowledge UncookedOnly is preferred

Copy link
Author

@olegkaspersky olegkaspersky Nov 17, 2024

Choose a reason for hiding this comment

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

Do we really want to be able to interact with git anyhow outside of the editor?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, there's an expectation that source control module is available outside of the editor in many Unreal tools from Epic

Copy link
Author

@olegkaspersky olegkaspersky Nov 17, 2024

Choose a reason for hiding this comment

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

just out of curiosity, could you give me an example because I couldn't find any in the engine folder
or you are talking about Borealis internal tools?

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.

3 participants