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

🚚 Rename add! and rm! functions for StandardModel #298

Merged
merged 6 commits into from
Jun 7, 2021

Conversation

stelmo
Copy link
Collaborator

@stelmo stelmo commented Jun 5, 2021

Currently CoreModel uses e.g. remove_reaction and add_reaction etc. However, StandardModel uses add! and rm!, this adds too much variability - the same method names should apply. Here StandardModel's functions are renamed.

However, a slight issue remains that the functions are in place for StandardModel, hence they have the bang!, while for CoreModel they make a new model and are thus not in place. The user thus needs to remember to use the bang for StandardModels and not for CoreModels. This is slightly non-optimal... what do you think?

@stelmo stelmo added the quality improves maintainability and code clarity label Jun 5, 2021
@stelmo stelmo requested a review from exaexa June 5, 2021 20:10
@stelmo stelmo changed the title 🚚 Rename add! and rm functions for StandardModel 🚚 Rename add! and rm functions for StandardModel Jun 5, 2021
@stelmo stelmo changed the title 🚚 Rename add! and rm functions for StandardModel 🚚 Rename add! and rm! functions for StandardModel Jun 5, 2021
@stelmo stelmo linked an issue Jun 5, 2021 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Jun 5, 2021

Codecov Report

Merging #298 (c082405) into master (4d87f7b) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #298      +/-   ##
==========================================
- Coverage   89.80%   89.74%   -0.07%     
==========================================
  Files          49       49              
  Lines        1138     1131       -7     
==========================================
- Hits         1022     1015       -7     
  Misses        116      116              
Impacted Files Coverage Δ
src/reconstruction/StandardModel.jl 92.00% <100.00%> (-0.99%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a9bdd4...c082405. Read the comment docs.

@stelmo
Copy link
Collaborator Author

stelmo commented Jun 5, 2021

closes #281

src/reconstruction/StandardModel.jl Outdated Show resolved Hide resolved
test/analysis/knockouts.jl Outdated Show resolved Hide resolved
@exaexa exaexa merged commit 40547bd into master Jun 7, 2021
@exaexa exaexa deleted the mo-stdmodel-mods branch June 7, 2021 07:24
laurentheirendt pushed a commit to laurentheirendt/COBREXA.jl that referenced this pull request Jun 8, 2021
🚚 Rename `add!` and `rm!` functions for `StandardModel`

Former-commit-id: 40547bd
exaexa added a commit that referenced this pull request Jul 8, 2021
🚚 Rename `add!` and `rm!` functions for `StandardModel`

Former-commit-id: 40547bd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quality improves maintainability and code clarity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Homogenize add/remove functions and add in place versions
2 participants