New syntax in defcomp for datum type parameterization

[corakingdon]

[Some of the CI tests on this PR will still fail, and we'll need to wait for the other PR that fixes them before we can merge this one]

This PR implements the new syntax for datum type parameterization in @defcomp, and errors when a user tries to use the old syntax. Do we want it to just be a deprecation warning instead of an error?

Addresses issue #473

[rjplevin]

Looks great!

[corakingdon]

@davidanthoff @lrennels

The tests for this PR are failing because it changes functionality that FUND relies on. (This changes the syntax from par1::Int = Parameter() to par1 = Parameter{Int}() in @defcomp). One of our tutorials uses FUND, so the doctests fail with this change. What would the proper work flow for this be?

We had removed the model dependency tests out of the normal tests required to merge PRs for exactly this reason, so that we can actually make breaking changes to Mimi master that might break the dependent models, with the intention that we would clean up the models once we are ready to release a new version of Mimi. I guess we forgot that our tutorials use the models as well.

[lrennels]

Ah interesting yea it's another one of these circular problems. I guess this should be fairly rare since it will happen only with breaking changes, but there are a few of those for v1.0.0.

We could remove the doctest for now, since I don't think we want to allow failing tests to be merged. We could also move doctests outside of the main testing suite, but that seems a bit dangerous to me because we'd have to make sure to double check those whenever we push to master and I'd prefer them be in the main testing suite.

I assume we'll change FUND quickly too, since the testing of our dependencies will break, so it's a temporary issue, @davidanthoff do you have a preference for how we handle this?

[davidanthoff]

Can we change the PR such that the old syntax is also still supported, with a deprecation warning?

[corakingdon]

Can we change the PR such that the old syntax is also still supported, with a deprecation warning?

Yes, that would work. I can make it just a warning for now, and then change it to breaking for v1.0.

[corakingdon]

@lrennels you are on it! I just went to add this to the issue with the list of deprecations-to-errors for v1.0 and you beat me to it! thanks!

[codecov]

Codecov Report

Merging #638 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #638   +/-   ##
=======================================
  Coverage   80.21%   80.21%           
=======================================
  Files          39       39           
  Lines        2745     2745           
=======================================
  Hits         2202     2202           
  Misses        543      543

Impacted Files	Coverage Δ
src/core/time.jl	97.26% <100%> (ø)	⬆️

---

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 e756fc6...0ff0cb9. Read the comment docs.

[lrennels]

@ckingdon95 looks good nice job getting into the weeds of macros I'm impressed! Did you check if there was any documentation we needed to change? I'm guessing yes since I saw you edited the tutorials but we should double check the docs too.

[corakingdon]

@lrennels good catch! I did actually miss one spot in the docs, I just fixed that. This makes me realize, though, that we never actually fully explain the different type parameterization options in the docs, and we probably should. I'll try to add something later today, probably just to the user guide!

[corakingdon]

@lrennels I just added a new section under Advanced Topics in the userguide, but I feel like I didn't do a great job explaining it. If you have any suggestions for better wording or examples let me know!