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

Pluralized macros return tuples #2844

Merged
merged 6 commits into from
Jan 9, 2022
Merged

Conversation

MartinBonde
Copy link
Contributor

@MartinBonde MartinBonde commented Jan 4, 2022

Closes #2837
Pluralized macros (e.g. @variables) now return vectors of whatever the 'singular' macros return.

E.g. the following new tests pass:
vars = @variables(model, begin
x
y[1:2]
end)
@test vars == [x, y]
eqs = @Constraints(model, begin
E_x, x == 0
E_y[i=1:2], y[i] == 0
end)
@test eqs == [E_x, E_y]

Pluralized macros (e.g. @variables) now return vectors of whatever the 'singular' macros return.

E.g. the following new tests pass:
    vars = @variables(model, begin
        x
        y[1:2]
    end)
    @test vars == [x, y]
    eqs = @Constraints(model, begin
        E_x, x == 0
        E_y[i=1:2], y[i] == 0
    end)
    @test eqs == [E_x, E_y]
@MartinBonde MartinBonde changed the title Pluralized macros now return vectors (issue #2837) Pluralized macros now return vectors Jan 4, 2022
@mlubin
Copy link
Member

mlubin commented Jan 4, 2022

Tuples seem like a better choice than vectors in this case. They'll give the compiler a chance at inferring the types of the elements if the situation permits. See https://discourse.julialang.org/t/when-to-use-tuples/18675.

@MartinBonde
Copy link
Contributor Author

I have no issue with using tuples instead.

@MartinBonde MartinBonde changed the title Pluralized macros now return vectors Pluralized macros return tuples Jan 4, 2022
test/macros.jl Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 4, 2022

Codecov Report

Merging #2844 (7e60976) into master (ee1bc2f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2844   +/-   ##
=======================================
  Coverage   94.78%   94.78%           
=======================================
  Files          44       44           
  Lines        5695     5698    +3     
=======================================
+ Hits         5398     5401    +3     
  Misses        297      297           
Impacted Files Coverage Δ
src/macros.jl 95.82% <100.00%> (ø)
src/Containers/DenseAxisArray.jl 90.36% <0.00%> (+0.13%) ⬆️

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 ee1bc2f...7e60976. Read the comment docs.

@odow
Copy link
Member

odow commented Jan 4, 2022

The documentation is failing because there are a lot of new printing changes now that nothing is no longer returned.

The easiest step is probably to add a semi-colon to the plural usages that appear in the documentation. This is probably a good reason to update the documentation to explain the new return value.

@odow odow merged commit b6db00a into jump-dev:master Jan 9, 2022
@odow
Copy link
Member

odow commented Jan 9, 2022

Thanks @MartinBonde

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.

Make plural macros (e.g. @variables) return collections of the defined objects
3 participants