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

Fix substring modifier implementation #1580

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DocMoebiuz
Copy link
Collaborator

@DocMoebiuz DocMoebiuz commented Jan 9, 2024

fixes #1558

This PR corrects the behavior of substring method. A substring has a Start, and has an End value.
The old behavior: End value was not included. The substring "starts" at Start and "ends before" End.
The new behavior: End value is included. The substring "starts" at Start and "ends on" End.

  • Substring Start and End mark the characters that will be included with the result
  • Add unit tests

Note

this PR also makes sure that the ConnectorValue is properly cloned and used as the result.

Copy link

github-actions bot commented Jan 9, 2024

Build for this pull request:
MobiFlightConnector.zip

@DocMoebiuz DocMoebiuz requested a review from neilenns January 10, 2024 22:21
@neilenns
Copy link
Contributor

What happens to people who have existing modifiers? Do they get auto-migrated to match this new behaviour? Or does everyone have to go and manually clean them up?

@DocMoebiuz
Copy link
Collaborator Author

i know. it's not ideal. but i have no good idea to tell whether it's an old config or not.

@neilenns
Copy link
Contributor

i know. it's not ideal. but i have no good idea to tell whether it's an old config or not.

All we really need to know is if this specific modifier is old or new. You can add a specific property to just the modifier object to identify that, something like endson=true. If it's missing then you know it's the old style and can auto-update it so it doesn't break the config, and then mark the file as dirty. On save, add the endson=true (or whatever flag name/value you want).

We had a LOT of problems in discord with the change to reverse digits. We should try and find a way to avoid the same thing happening again with this change :)

@DocMoebiuz
Copy link
Collaborator Author

I totally agree.

I don't know how many are using this. It was only for FSUIPC string type in the past.

I thought of an extra attribute too and I guess that's the easiest option.

I will add it.

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.

Substring modifier returns one less character
2 participants