FIX: Fixed Input Actions code generation overwriting user files when the names happened to match. #2159
Conversation
… input actions target
K-Tone
changed the title
ritamerkl
left a comment
ritamerkl
left a comment
There was a problem hiding this comment.
Just a small nitpick, otherwise looks good! Nice to have this!
| if (File.Exists(wrapperFilePath)) | ||
| { | ||
| var text = File.ReadAllText(wrapperFilePath); | ||
| var autoGeneratedMarker = "This code was auto-generated by com.unity.inputsystem"; |
There was a problem hiding this comment.
I was wondering if we can utilise the string the auto generator uses as well, to avoid that this breaks in the case the string would change. I am not sure how much overhead this would entail, I am comfortable with this solution too.
There was a problem hiding this comment.
Thanks for the fast review!
This is a good point so I've checked code around just a little bit. Appears, the code generator template is declared as a constant inside a static function. It's used in just two spots, both of which specialise it with some extra data. Regardless of the specialization, it's going to include this particular string that we have right now on the PR. It feels immanent to the code logics that we would include the package name as part of it always.
Looking at it, it feels unlikely it's going to be broken any time soon. We would need to add another code generator. I guess, it's indeed more risk in the form of extra code churn if we were to use (portions) of the actual template used by generator.
There was a problem hiding this comment.
Thanks for checking, that is fine for me!
ekcoh
left a comment
ekcoh
left a comment
There was a problem hiding this comment.
Fix makes sense and seems to match the description.
|
Restarted CI run since I believe the failure was just noise, CHANGELOG conflict needs to be fixed though. |
Pauliusd01
left a comment
Pauliusd01
left a comment
There was a problem hiding this comment.
LGTM, here's what I checked:
- User repro, when moving the input actions asset to a new folder and renaming
- Whether error still shows up when regenerating the class (uncheck recheck the option)
- Input actions asset itself is preserved correctly when failing to generate the code
- Whether code generation works when it finds auto generated files with the same name like the error message describes. (I copied over info from a different asset with different names and it did overwrite it)
- Does generated code update correctly when updating the Input Actions asset
Description
Apparently, if a user project happens to already have a script with the same name as was also chosen in the Input Actions importer as the target C# file, we would silently overwrite it with auto-generated stuff and so the user might loose data.
In the comments to the case it was suggested to check if the file exists, and if it does - then only overwrite when we're sure that it's whatever we generated before. Otherwise, output an error. So that's precisely what I did here.
Testing status & QA
Local testing, added the area QA.
Overall Product Risks
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.After merge: