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

Add mod table setting for specifying targetted FSO version #1414

Merged
merged 1 commit into from Aug 24, 2017

Conversation

Projects
None yet
4 participants
@asarium
Member

asarium commented Jul 13, 2017

This adds a small system for letting a mod specify which version of FSO
was designed for. That would allow us to introduce code which would
normally break backwards-compatibility by putting it in an if block
which checks if the mod supports the FSO version which ontroduced the
behavioral change.

There is currently no code which uses the new functions yet so I added a
few tests to verify that the options do what they are designed to do.

This fixes #1389.

@asarium asarium added the enhancement label Jul 13, 2017

@asarium asarium added this to the Release 3.8.2 milestone Jul 13, 2017

Show outdated Hide outdated code/globalincs/version.cpp
Add mod table setting for specifying targetted FSO version
This adds a small system for letting a mod specify which version of FSO
was designed for. That would allow us to introduce code which would
normally break backwards-compatibility by putting it in an if block
which checks if the mod supports the FSO version which ontroduced the
behavioral change.

There is currently no code which uses the new functions yet so I added a
few tests to verify that the options do what they are designed to do.

This fixes #1389.
@MageKing17

This comment has been minimized.

Show comment
Hide comment
@MageKing17

MageKing17 Jul 15, 2017

Member

Nothing else jumps out at me.

Member

MageKing17 commented Jul 15, 2017

Nothing else jumps out at me.

@Goober5000

This comment has been minimized.

Show comment
Hide comment
@Goober5000

Goober5000 Jul 15, 2017

Contributor

Although this is categorized as a feature, I think it would make sense to merge it into 3.8.0. It would help for as many versions of FSO as possible to recognize this flag even if no code specifically checks for it yet.

Contributor

Goober5000 commented Jul 15, 2017

Although this is categorized as a feature, I think it would make sense to merge it into 3.8.0. It would help for as many versions of FSO as possible to recognize this flag even if no code specifically checks for it yet.

@asarium

This comment has been minimized.

Show comment
Hide comment
@asarium

asarium Jul 15, 2017

Member

The flag is already being recognized as "$Minimum version:". I just added an alias for that option to better fit the functionality of the feature.

I think it would be a bad idea to merge this now. We are already at RC3 which may be the build that becomes the final release. Merging this code which touches some existing functionality may break stuff in unforeseen ways and that should not be done in an RC phase, especially not if there were already 3 RCs.

Member

asarium commented Jul 15, 2017

The flag is already being recognized as "$Minimum version:". I just added an alias for that option to better fit the functionality of the feature.

I think it would be a bad idea to merge this now. We are already at RC3 which may be the build that becomes the final release. Merging this code which touches some existing functionality may break stuff in unforeseen ways and that should not be done in an RC phase, especially not if there were already 3 RCs.

@The-E

This comment has been minimized.

Show comment
Hide comment
@The-E

The-E Jul 15, 2017

Member

Agreed. This is a 3.8.2 feature.

Member

The-E commented Jul 15, 2017

Agreed. This is a 3.8.2 feature.

@Goober5000

This comment has been minimized.

Show comment
Hide comment
@Goober5000

Goober5000 Jul 15, 2017

Contributor

Well, I would still suggest a separate PR just for the $Target Version alias even if none of the other functionality is included.

Contributor

Goober5000 commented Jul 15, 2017

Well, I would still suggest a separate PR just for the $Target Version alias even if none of the other functionality is included.

@MageKing17

This comment has been minimized.

Show comment
Hide comment
@MageKing17

MageKing17 Jul 16, 2017

Member

To be honest, I'd rather not add the alias at all than slip it into 3.8.

Member

MageKing17 commented Jul 16, 2017

To be honest, I'd rather not add the alias at all than slip it into 3.8.

@asarium

This comment has been minimized.

Show comment
Hide comment
@asarium

asarium Jul 16, 2017

Member

The alias made sense to me because "Minimum version" may mean the same thing but "Targeted Version" somehow seemed to be a more intuitive name, at least for me.

Member

asarium commented Jul 16, 2017

The alias made sense to me because "Minimum version" may mean the same thing but "Targeted Version" somehow seemed to be a more intuitive name, at least for me.

@MageKing17 MageKing17 merged commit f1101be into scp-fs2open:master Aug 24, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@asarium asarium deleted the asarium:feature/targettedFSOVersion branch Aug 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment