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

Clean up JuMP.Model. #1383

Merged
merged 1 commit into from
Jul 29, 2018
Merged

Clean up JuMP.Model. #1383

merged 1 commit into from
Jul 29, 2018

Conversation

IainNZ
Copy link
Collaborator

@IainNZ IainNZ commented Jul 28, 2018

Remove unused fields, style guide, initial attempts to document from
an outsiders perspective - very much WIP.

Also an attempt to start matching TODOs with issues/people.

@IainNZ
Copy link
Collaborator Author

IainNZ commented Jul 28, 2018

Need to rebase on top of #1362

@codecov

This comment has been minimized.

@IainNZ
Copy link
Collaborator Author

IainNZ commented Jul 28, 2018

Rebase done.

src/JuMP.jl Outdated
# Model
################################################################################
# Model.
# TODO(IainNZ): Clarify the three keyword arguments. `mode` is explained,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is being refactored in #1384

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should hold off merging this for now or quickly get it in and build from it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you revert the docstring for Model and the TODO right here that's going to be out of date? Everything else seems fine.

"""
objectivevalue(m::Model) = MOI.get(m, MOI.ObjectiveValue())
objectivevalue(model::Model) = MOI.get(model, MOI.ObjectiveValue())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We haven't yet moved over the naming discussion to the style guide, but what do you think about objective_value etc for these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a fan of underscores (another difference from Base, but w/e...), so sgtm

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's hold off on renaming until we resolve the style question, otherwise it might just be unnecessary churn.

Remove unused fields, style guide, initial attempts to document from
an outsiders perspective - very much WIP.

Also an attempt to start matching TODOs with issues/people.
@IainNZ
Copy link
Collaborator Author

IainNZ commented Jul 28, 2018

Done

@mlubin mlubin merged commit 64fc678 into master Jul 29, 2018
@mlubin mlubin deleted the docmodel branch July 29, 2018 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants