Plugin Script Rewrite #862
Conversation
|
|
||
| if game:GetService("RunService"):IsRunning() then | ||
| local IsRunning = RunService:IsRunning() |
There was a problem hiding this comment.
I don't really understand the point in putting this into it's own variable, the original seemed fine
| local connected = Instance.new("BoolValue") | ||
| local connections = {} | ||
| local DefaultSettingsModule = Instance.new("ModuleScript") | ||
| DefaultSettingsModule.Name = SETTINGS_MODULE_NAME |
There was a problem hiding this comment.
What is the point in inlining this script contents and then creating an instance. Was something wrong with the predefined DefaultSettings.lua? This seems like a roundabout way to end up just doing the same thing that was there before.
| Settings.Port = DefaultSettings.Port | ||
| elseif type(Settings.StartAutomatically) ~= "boolean" then | ||
| warn( | ||
| `[Luau Language Server] Could not load settings: StartAutomatically is not a boolean. Defaulting to {DefaultSettings.Port}.` |
There was a problem hiding this comment.
Should this be DefaultSettings.StartAutomatically?
| plugin:OpenScript(SettingsModule) | ||
| wasConnected = connected.Value | ||
| end) | ||
| return SettingsModule |
There was a problem hiding this comment.
We perform edits to Settings but end up just returning SettingsModule, won't this discard all the edits we made?
| encoded.Name = instance.Name | ||
| encoded.ClassName = instance.ClassName | ||
| encoded.Children = {} | ||
| local function EncodeInstance(Instance: Instance, ChildFilter: ((Instance) -> boolean)?): EncodedInstance |
There was a problem hiding this comment.
Any particular reason to switch the casing of all these variables?
In particular, Instance clashes with the global Instance library, please change this.
| encoded.Children = {} | ||
| local function EncodeInstance(Instance: Instance, ChildFilter: ((Instance) -> boolean)?): EncodedInstance | ||
| local Encoded = {} | ||
| Encoded.Name = Instance.Name or "EmptyName" |
There was a problem hiding this comment.
Why do we set or "EmptyName" and or "EmptyClassName" here? AIUI, these should always be set for a given instance. EmptyClassName in particular might break the server since it's an invalid class name.
| local function SetupListeners(Instance: Instance) | ||
| local NameChangedConnection: RBXScriptConnection = nil | ||
| NameChangedConnection = Instance:GetPropertyChangedSignal("Name"):Connect(function() | ||
| print(Settings.DebugMode) |
| SettingsModule.Parent = TestService | ||
|
|
||
| local Settings = require(SettingsModule) :: any | ||
| print(Settings) |
There was a problem hiding this comment.
Should we be printing this?
| ParentChangedConnection:Disconnect() | ||
| NameChangedConnection:Disconnect() |
There was a problem hiding this comment.
Why do we disconnect these, Instance might still exist right? So we'll lose any further changes to it
Bug fixes
Implemented features
Planned features (future PRs, other collaborators)