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

chore: move deprecated scripts #209

Merged
merged 4 commits into from
Mar 24, 2020
Merged

chore: move deprecated scripts #209

merged 4 commits into from
Mar 24, 2020

Conversation

BenjaSanchez
Copy link
Contributor

@BenjaSanchez BenjaSanchez commented Mar 20, 2020

Main improvements in this PR:

Moved deprecated files to separate folder to avoid confusion. Also clarified with README's the purpose of each script/data folder.

I hereby confirm that I have:

  • Tested my code with all requirements for running the model
  • Selected devel as a target branch (top left drop-down menu)
  • If needed, asked first in the Gitter chat room about this PR

@BenjaSanchez BenjaSanchez added the doc documentation label Mar 20, 2020
@BenjaSanchez BenjaSanchez requested a review from feiranl March 20, 2020 09:18
@BenjaSanchez BenjaSanchez self-assigned this Mar 20, 2020
@BenjaSanchez BenjaSanchez requested a review from edkerk March 24, 2020 10:10
@edkerk
Copy link
Member

edkerk commented Mar 24, 2020

Do we have somewhere documented which script was used for which model version? This could be dealt with by having subfolders like deprecated/sincev8.1.0/, deprecated/sincev8.1.1 etc. Perhaps not stricly required, if we consistenly move old scripts to the deprecated folder when making a new release as soon as a particular script is no longer required, and mention this explicitly in the PR message.

@BenjaSanchez
Copy link
Contributor Author

BenjaSanchez commented Mar 24, 2020

@edkerk thanks for your feedback!

Do we have somewhere documented which script was used for which model version?

Good point, we don't. I will document it here for all the ones that were moved in this PR:

Folder File Model Version
modelCuration addiSce926changes.m yeast 8.0.0
fixBiomassComp.m yeast 8.1.0
makeFormulasCompliant.m yeast 8.1.0
modelCorrections.m yeast 7.7.0
takeOutFromFormula.m yeast 8.1.0
updateMetaboliteFormula.m yeast 8.3.4
otherChanges addSLIMErxns.m yeast 8.1.0
clusterBiomass.m yeast 7.8.0
convertYmn2FBC2.m yeast 7.7.0

[...] if we consistenly move old scripts to the deprecated folder when making a new release as soon as a particular script is no longer required, and mention this explicitly in the PR message.

I think this solution is preferable, to avoid having a very complex sub-folder structure. We can also mention it in the commit description for all new migrations.

@edkerk edkerk merged commit d0b4e0b into devel Mar 24, 2020
@edkerk edkerk deleted the chore/move-deprecated-scripts branch March 24, 2020 21:03
@BenjaSanchez BenjaSanchez mentioned this pull request Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants