feat: Created a converter for the AttachmentIdentifier structure and a few other minor changes#799
feat: Created a converter for the AttachmentIdentifier structure and a few other minor changes#799PUDGE133 wants to merge 7 commits intoExMod-Team:masterfrom
Conversation
- Deleted incorrect error method "AttachmentIdentifier.TryParse" - Renamed "AttachmentName" converter because its name was incorrect and interfered with the new converter - Created a new converter "Attachment Identifier Converter" for the "Attachment Identifier" structure - Implemented new changes to the Loader class
Co-authored-by: Yamato <66829532+louis1706@users.noreply.github.com>
|
What's taking so long? I'm ready to do a new PR on a different issue, but I'm afraid the changes will come right here. |
|
@PUDGE133 Shouldn't you just create a new branch that targets master (or dev if breaking changes ig) to do the new PR? And, I don't want to be the annoying guy but they have their own stuff to do so, don't say |
I understand they have their own things to do. I'm just not very familiar with the git system and have no idea what actions might lead to what. |
Then search abt it, if you make a mistake it happens, and just move on If you want a really easy experience, just use Github Desktop, should be easier to manage your branches |
| /// <summary> | ||
| /// Converts the string representation of a <see cref="AttachmentIdentifier"/> to its <see cref="AttachmentIdentifier"/> equivalent. | ||
| /// A return value indicates whether the conversion is succeeded or failed. | ||
| /// </summary> | ||
| /// <param name="s">The <see cref="string"/> to convert.</param> | ||
| /// <param name="identifier">The converted <see cref="string"/>.</param> | ||
| /// <returns><see langword="true"/> if <see cref="string"/> was converted successfully; otherwise, <see langword="false"/>.</returns> | ||
| public static bool TryParse(string s, out AttachmentIdentifier identifier) | ||
| { | ||
| identifier = default; | ||
|
|
||
| foreach (AttachmentIdentifier attId in Features.Items.Firearm.AvailableAttachments.Values.SelectMany(kvp => kvp.Where(kvp2 => kvp2.Name.ToString() == s))) | ||
| { | ||
| identifier = attId; | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
This is a breaking change. Either keep this method here or change the target branch to Dev (which will only be released when Exiled 10 comes out)
There was a problem hiding this comment.
The existence of this method itself is a critical bug, not the fact that I removed it in this PR. https://discord.com/channels/656673194693885975/1002713309876854924/1488609613258559681 There's not a single reference to this method in the code. It also seems unlikely that anyone has ever used it. Do you really want to keep this bug around until the next version of the game is released?
There was a problem hiding this comment.
Sorry for taking so long to respond to this. But to answer your question, yes I do want to keep this even if it's buggy as some other plugin could have used this method and then that entire plugin would throw a MissingMethodException whenever it tries to call this
| /// Converts a <see cref="IEnumerable{T}"/> of <see cref="AttachmentName"/> to Yaml configs and vice versa. | ||
| /// </summary> | ||
| public sealed class AttachmentIdentifiersConverter : IYamlTypeConverter | ||
| public sealed class AttachmentNameConverter : IYamlTypeConverter |
There was a problem hiding this comment.
This is another breaking change. Albeit more minor
There was a problem hiding this comment.
This doesn't affect anything at all. Not only was the name incorrect, it also got in the way of developing a new feature.
There was a problem hiding this comment.
If any Exiled plugin whatsoever used this converter in their own code, after this change they'd get a TypeLoadException. So I'm not approving of this PR until it's either set to target Dev, or if these breaking changes are removed
|
Well... then we'll wait until the new patch comes out. |
4 participants
Description
What is the current behavior? (You can also link to an open issue here)
Everything works great.
What is the new behavior? (if this is a feature change)
The AttachmentIdentifier object is now correctly serialized and deserialized in the config.
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
There are no critical changes. I removed a broken method that no sane person would use. I overrode the default serialization, which also won't do any harm, because deserialization didn't work, meaning no one was using it, and it won't break any configs.
Other information:
Types of changes
Submission checklist
Patches (if there are any changes related to Harmony patches)
Other