Merge Variables and DatedVariables #522

sandcha · 2017-06-01T07:45:12Z

Breaking changes

In variables:
- Remove SimpleVariable and DatedVariable. Any Variable acts like old DatedVariable
- Remove start_date attribute
- Rename stop_date attribute to end
In formulas:
- Remove SimpleFormula and DatedFormula. Any Formula acts like old DatedFormula
- Remove @dated_function: start goes to formula name suffix, stop is deduced from other formulas start dates and variable end attribute

New features

Introduce formula_YYYY[_MM[_DD]]
- Allows for formula start instant definition
Introduce end = 'YYYY-MM-DD'
- Allows for variable end date definition in string format
Change ETERNITY period effect
- Allows for dated Variable definition on ETERNITY period

Technical changes

In variables:
- Remove Abstract class
In formulas:
- Remove Abstract classes
In tests:
- Add test_variable

Fixes #371.

fpagnoux · 2017-06-01T08:44:38Z

Tests are passing in france 🎉

(Red only because the version has not been bumped)

fpagnoux

Nice !

I couldn't read the whole code change yet, but here are a few remarks.

fpagnoux · 2017-06-01T08:53:30Z

openfisca_core/columns.py

@@ -84,7 +84,7 @@ def empty_clone(self):
    def is_input_variable(self):
        """Returns true if the column (self) is an input variable."""
        from . import formulas
-        return issubclass(self.formula_class, formulas.SimpleFormula) and self.formula_class.function is None
+        return issubclass(self.formula_class, formulas.Formula) and self.formula_class.formula is None


I don't think the first part of the condition is relevant anymore.

Before, we had two kinds of formulas and we wanted to check it was a 'simple' one.
Now, self.formula_class is a Formula by construction.

fpagnoux · 2017-06-01T08:58:33Z

openfisca_core/holders.py

        if holder_or_dated_holder.array is not None:
            return holder_or_dated_holder
        assert self._array is None  # self._array should always be None when dated_holder.array is None.

        # Request a computation
-        formula_dated_holder = self.formula.compute(period = period, **parameters)
+        dated_holder = self.formula.compute(period = period, **parameters)
+        assert isinstance(dated_holder, DatedHolder)


Adding assertion trough the code is useful for debugging, but must be avoided in prod.

If the assertion is not respected, an AssertionError will be thrown, without any explanation.

Here, as by construction compute always return a DatedHolder, I think we can just remove the assertion.

fpagnoux · 2017-06-01T08:59:29Z

openfisca_core/holders.py

-                ][0]['formula'].function
-        else:
-            formula = self.formula.function
+        formula = self.formula.find_function(period)


I'd expect the output of self.formula.find_function to be named function.

fpagnoux · 2017-06-01T09:03:35Z

openfisca_core/variables.py

@@ -34,7 +34,7 @@ def get_introspection_data(self, tax_benefit_system):
        return comments, source_file_path, source_code, start_line_number

    def to_column(self, tax_benefit_system):
-        formula_class = self.__class__.formula_class
+        formula_class = self.__class__.formula_class  # Formula


Since you simplified everything, formula_class is always Formula !

We should:

remove the class attribute formula_class = Formula

remove formula_class for new_filled_column arguments as it's always the same.

fpagnoux · 2017-06-01T09:03:55Z

openfisca_core/variables.py

-class AbstractVariable(object):
-    formula_class = None
+class Variable(object):
+    formula_class = Formula


Remove this class attribute (see comment below).

formula_class removed from Variable but maintained as item of Formula.dated_formulas_class

fpagnoux

More remarks.

Your attention on quality and testing will be very useful for OpenFisca 😎

However, I think we have too many tests here. The goal of unit testing is not to make sure every combination works (it's not realistic). We can talk about it IRL :)

fpagnoux · 2017-06-01T15:17:17Z

tests/core/test_variable.py

+from openfisca_core.periods import MONTH, ETERNITY
+from openfisca_core.columns import IntCol
+
+import openfisca_dummy_country as dummy_country


The dummy country is deprecated, we should use the country template instead.

fpagnoux · 2017-06-01T15:23:21Z

tests/core/test_variable.py

+    variable = tax_benefit_system.column_by_name['variable__no_dated_attributes']
+    assert variable is not None
+    assert variable.end is None
+    assert variable.formula_class.dated_formulas_class.__len__() == 0


To get the length of an array in Python, we usually use len(variable.formula_class.dated_formulas_class).

__len__ is a magic method. It's usually used only to define what len does on a given object you create.

fpagnoux · 2017-06-01T15:28:15Z

tests/core/test_variable.py

+add_variable_catch_assertion(tax_benefit_system, variable__deprecated_start_date, 'Deprecated "start_date" attribute in definition of variable')
+
+
+# 371 - end, no function


Why mentioning the number of the PR here ?

As a lazy way to find history. Removed.

fpagnoux · 2017-06-01T15:38:23Z

tests/core/test_variable.py

+        raise
+
+    # Check that Error at variable adding prevents it from registration in the taxbenefitsystem.
+    assert not hasattr(tax_benefit_system.column_by_name, "variable__strange_end_attribute")


That's not what you want to test ;).

column_by_name = {'a' : 1} hasattr(column_by_name, 'a') >>> False

hasattr check if tax_benefit_system.column_by_name has an attribute named variable__strange_end_attribute . An attribute is something you get with the column_by_name.a notation. It's not the same thing than an item you get with column_by_name['a'].

You could use :

column_by_name = {'a' : 1} column_by_name.has_key('a') >>> True

But the most idiomatic is :

column_by_name = {'a' : 1} column_by_name.get('a') >>> 1 column_by_name.get('b') >>> None assert not column_by_name.get('b')

There is a slight difference between the 2 solutions: the case where a property as been explicitly set to None, which is not relevant in our case.

fpagnoux · 2017-06-01T15:40:50Z

tests/core/test_variable.py

+    entity = Individu
+    definition_period = MONTH
+    label = u"Variable with end attribute, no function."
+    # start_date = datetime.date(1980, 1, 1)


Why having this commented line ?

fpagnoux · 2017-06-01T15:53:42Z

tests/core/test_variable.py

+    entity = Individu
+    definition_period = MONTH
+    label = u"Variable with end attribute, one function without date."
+    # start_date = datetime.date(1980, 1, 1)


Commented line

fpagnoux · 2017-06-01T15:55:43Z

tests/core/test_variable.py

+def test_call__dated_attribute__one_formula():
+    month = '1979-12'
+    simulation = new_simulation(tax_benefit_system, month)
+    assert simulation.calculate('dated_attribute__one_formula', month, extra_params=[100]) == 100


I'm not sure using extra_params make this test clearer. Why not just write return vectorize(self, 100) ?

fpagnoux · 2017-06-01T15:56:25Z

tests/core/test_variable.py

+    assert variable is not None
+    assert variable.end == '1989-12-31'
+
+    assert variable.formula_class.dated_formulas_class.__len__() == 1


fpagnoux · 2017-06-01T15:57:58Z

tests/core/test_variable.py

+def test_dates__dated_attribute__one_formula():
+    variable = tax_benefit_system.column_by_name['dated_attribute__one_formula']
+    assert variable is not None
+    assert variable.end == '1989-12-31'


Same remark than above: you already checked that for a variable declared with an end attribute, the end can be retrieved with variable.end. This assertion is redundant.

fpagnoux · 2017-06-01T16:02:50Z

tests/core/test_variable.py

+    assert variable.formula_class.dated_formulas_class.__len__() == 1
+    formula = variable.formula_class.dated_formulas_class[0]
+    assert formula is not None
+    assert formula['start_instant'].date == datetime.date(1, 1, 1)


datetime.date(1, 1, 1) should be replaced by datetime.date.min.
If tomorrow datetime is adapted to handle the egyptian era , we don't want to impose ourselves to stop at 0001-01-01 :)

Ok for tests. And it will show us the egyptian era case when it comes as datetime.date.min is not applied in formulas.py (to be consistent with formula name specifications).

fpagnoux

Great test coverage!

fpagnoux · 2017-06-06T09:19:02Z

openfisca_core/formulas.py

+    if end is UnboundLocalError:
+        end = None if reference_column is None else reference_column.end
+    elif end is not None:
+        assert isinstance(end, str)


I know this was the case before, but we must never add assertion without an error message.
If in my code I write:

class basic_income(Variable): column = FloatCol entity = Person definition_period = MONTH label = "Basic income provided to adults" url = "https://law.gov.example/basic_income" # Always use the most official source end = date(2015, 1, 2)

I get the error:

assert isinstance(end, str) AssertionError:

As a formula writer, I have no idea what to do.

fpagnoux · 2017-06-07T09:11:27Z

openfisca_core/formulas.py

+            + formula_default_day
+
+    else:
+        match = re.match(r'(formula_)(\d{4}(_\d{2}){0,2})', formula_name)  # YYYY or YYYY_MM or YYYY_MM_DD


This regexp is not strict enough. It matches formula_2015_toto for instance.

As a formula writer, if I write:

def formula_2015_toto(person, period, legislation):

I get the error:

ValueError: time data 'formula_2015_toto_01_01' does not match format 'formula_%Y_%m_%d'

which is not as explicit as the one you wrote :)

Also, are the parenthesis around formula_ necessary ?

fpagnoux · 2017-06-07T13:17:09Z

openfisca_core/columns.py

@@ -83,8 +83,7 @@ def empty_clone(self):

    def is_input_variable(self):
        """Returns true if the column (self) is an input variable."""
-        from . import formulas
-        return issubclass(self.formula_class, formulas.SimpleFormula) and self.formula_class.function is None
+        return self.formula_class.formula is None


This doesn't work.

type object 'accomodation_size' has no attribute 'formula'

Should probably be

return self.formula_class.dated_formulas_class == []

fpagnoux · 2017-06-07T13:23:15Z

openfisca_core/formulas.py

@@ -69,10 +82,21 @@ def clone(self, holder, keys_to_skip = None):
            if key not in keys_to_skip:
                new_dict[key] = value

+        keys_to_skip.add('dated_formulas')


We already filtered the values in line 82. Adding dated_formulas at this point doesn't do anything.

fpagnoux · 2017-06-07T13:32:11Z

openfisca_core/formulas.py

+
+    def find_function(self, period):
+        """
+        This function finds the last active formula for the time interval [period starting date, variable end attribute].


This function is not necessary, we know it's a function ;).
-->
Finds the last...

Done (it was used to act like the python official documentation but it's not even consistent there).

fpagnoux · 2017-06-07T14:39:57Z

tests/core/test_variable.py

+
+# end, one function without date
+
+class dated_attribute__one_simple_formula(Variable):


end_attribute...

fpagnoux · 2017-06-07T14:40:27Z

tests/core/test_variable.py

+
+def test_dates__dated_attribute__one_simple_formula():
+    variable = tax_benefit_system.column_by_name['dated_attribute__one_simple_formula']
+    assert variable is not None


Same than above, this cannot be None

fpagnoux · 2017-06-07T14:41:43Z

tests/core/test_variable.py

+
+    assert len(variable.formula_class.dated_formulas_class) == 1
+    formula = variable.formula_class.dated_formulas_class[0]
+    assert formula is not None


Could this really be None ?

Not really. Removed in file.

fpagnoux · 2017-06-07T14:43:26Z

tests/core/test_variable.py

+
+    i = 1
+    formula = variable.formula_class.dated_formulas_class[i]
+    assert formula is not None


Same than above: can this ever be None ?

fpagnoux · 2017-06-07T14:44:01Z

tests/core/test_variable.py

+    column = IntCol
+    entity = Person
+    definition_period = MONTH
+    label = u"Variable, no and attribute, multiple dated functions with different names and no date overlap."


fpagnoux · 2017-06-07T16:27:07Z

CHANGELOG.md

@@ -1,5 +1,35 @@
 # Changelog

+# 13.0.0


I copy-pasted what you wrote in the PR description to start discussing the Changelog :).

The general idea of the Changelog is to be written from the point of view of a OpenFisca Core user (ie someone who is writing their own legislation)

fpagnoux · 2017-06-07T16:28:06Z

CHANGELOG.md

+#### Breaking changes
+
+- In variables:
+  - Remove SimpleVariable and DatedVariable. Any Variable acts like old DatedVariable


I don't think there is currently anything named SimpleVariable.

Suggestion:

Merge Variable and DatedVariable

Variable can now handle formula evolution over time.

Ok for second line and the fanciful SimpleVariable.
In first line, what about AbstractVariable ? It was possible to create its own kind of variable before and we removed this service.

I don't think AbstractVariable was ever meant to be extended by child classes in country packages 🤔.
I don't think anyone ever did it anyway, so it's ok not to mention it.

fpagnoux · 2017-06-07T16:29:29Z

CHANGELOG.md

+
+- In variables:
+  - Remove SimpleVariable and DatedVariable. Any Variable acts like old DatedVariable
+  - Remove start_date attribute


Remove -> Deprecate and remove

Does a real deprecated attribute raise a warning and is still managed or does it raise a fatal error ?

Good question.
If we just "deprecate" something, I'd assume that it's still available and raise a waring.
If we "deprecate and remove" something, I'd assume it will raise a fatal error.
If we "remove" something, I'm just afraid it's not crystal clear that what we removed is a feature.

@MattiSG @michelbl @Anna-Livia thoughts ?

fpagnoux · 2017-06-07T16:30:58Z

CHANGELOG.md

+- In variables:
+  - Remove SimpleVariable and DatedVariable. Any Variable acts like old DatedVariable
+  - Remove start_date attribute
+  - Rename stop_date attribute to end


When you are quoting code (for instance stop_date), it's better to use backticks (`) to format it properly.

fpagnoux · 2017-06-07T16:33:13Z

CHANGELOG.md

+  - Rename stop_date attribute to end
+- In formulas:
+  - Remove SimpleFormula and DatedFormula. Any Formula acts like old DatedFormula
+  - Remove @dated_function: start goes to formula name suffix, stop is deduced from other formulas start dates and variable end attribute 


I'dd add an example for one formula to explicit the change.

Or one example with variable class definition and formulas to show linked changes?

fpagnoux · 2017-06-07T16:33:54Z

CHANGELOG.md

+  - Remove Abstract class
+- In formulas:
+  - Remove Abstract classes
+- In tests:


Adding tests is usually not something the users care about, so usually it's not necessary to mention it in the changelog.

fpagnoux · 2017-06-07T16:36:59Z

CHANGELOG.md

+#### Technical changes
+
+- In variables:
+  - Remove Abstract class


This information is probably not needed in the Changelog:

Users don't interact with these abstract classes.

Developers can read in the diff that you removed these classes

It seems to me that it's more a service that disappears than a technical change. So, should we really silence this information ?

I don't think AbstractVariable was ever meant to be extended by child classes in country packages 🤔.
I don't think anyone ever did it anyway, so it's ok not to mention it.

fpagnoux · 2017-06-07T16:38:58Z

CHANGELOG.md

+- Introduce `end = 'YYYY-MM-DD'`
+  - Allows for variable end date definition in string format
+- Change ETERNITY period effect
+  - Allows for dated Variable definition on ETERNITY period


Dated variables don't exist anymore, so mentioning them in the changelog is not great.

Suggestion :

Remove restriction that prevented to have formula changes over time for a variable with ETERNITY as a definition period.

fpagnoux · 2017-06-07T16:41:02Z

CHANGELOG.md

+
+#### New features
+
+- Introduce `formula_YYYY[_MM[_DD]]`


This replaces the former @dated_function decorator, so it should be mentioned near the its deprecation.

As a user, it's better for me to find in the same place what syntax has disappeared, and what has replaced it.

fpagnoux · 2017-06-07T16:41:43Z

CHANGELOG.md

+- Introduce `formula_YYYY[_MM[_DD]]`
+  - Allows for formula start instant definition
+- Introduce `end = 'YYYY-MM-DD'`
+  - Allows for variable end date definition in string format


Same: should be put near Rename stop_date attribute to end

fpagnoux · 2017-06-07T16:48:34Z

tests/core/test_variable.py

+    end = '1989-12-31'
+
+
+check_error_at_add_variable(tax_benefit_system, variable__deprecated_start_date, 'Deprecated "start_date" attribute in definition of variable')


Should be in a test

fpagnoux · 2017-06-09T10:23:33Z

openfisca_core/formulas.py

+        if end and period.start.date > get_datetime_date(self, end):
+            return None
+
+        i = len(self.dated_formulas)


This variable is not use anywhere anymore ;)

fpagnoux · 2017-06-09T10:25:32Z

openfisca_core/formulas.py

+        # Current attribute isn't a formula
+        return None
+
+    formula_name_separator = '_'


Why not declare all these constants at the same place than formula_name_prefix?

In case it exits with None, we would have unnecessary assignments execution.

If you worry about unnecessary assignments, make these constants and put them out of this function ;)

fpagnoux · 2017-06-09T10:31:33Z

openfisca_core/formulas.py

+    formula_default_day = '01'
+
+    if formula_name == formula_name_prefix:
+        formula_name += formula_name_separator + formula_default_year + formula_name_separator\


W503 is highly controversial and outdated.

I'm black-listing it right now 😄

fpagnoux · 2017-06-09T10:41:58Z

openfisca_core/formulas.py

+    formula_name_prefix + formula_name_separator + '%Y_%m_%d').date()
+
+
+def get_datetime_date(src_name, date_str):


This signature is not natural. Passing an argument just so that it is used in an error message is another sign that the code structure is not optimal.

fpagnoux · 2017-06-09T10:56:20Z

openfisca_core/formulas.py

+        end = None if reference_column is None else reference_column.end
+    elif end is not None:
+        assert isinstance(end, str)
+        get_datetime_date(name, end)  # Raises ValueError on format error.


I don't see how you would need to call this helper several times if you just parsed the string once and for all.
Careful, here, we are parsing the end attribute every single time we call find_function.

It seems much more natural to parse the input only once. The fact that we end up using a method named get_datetime_date just to raise an error is a clear sign that our code structure is lopsided.

fpagnoux · 2017-06-09T11:05:54Z

tests/core/test_variable.py

+    column = IntCol
+    entity = Person
+    definition_period = MONTH
+    label = u"Variable with dated attributes (including deprecated start_date), no formula."


u"Variable with and end attribute and a deprecated start_date, no formula."

fpagnoux · 2017-06-09T11:10:37Z

CHANGELOG.md

+#### Breaking changes
+
+- In variables:
+  - Remove SimpleVariable and DatedVariable. Any Variable acts like old DatedVariable


I don't think AbstractVariable was ever meant to be extended by child classes in country packages 🤔.
I don't think anyone ever did it anyway, so it's ok not to mention it.

fpagnoux

We're getting there :)

fpagnoux · 2017-06-09T11:14:59Z

CHANGELOG.md

+    - start definition goes to formula name: `formula_YYYY[_MM[_DD]]`
+    - stop is deduced from next formula start
+
+  Before:


fpagnoux · 2017-06-09T11:16:15Z

CHANGELOG.md

+#### Technical changes
+
+- In variables:
+  - Remove Abstract class


I don't think AbstractVariable was ever meant to be extended by child classes in country packages 🤔.
I don't think anyone ever did it anyway, so it's ok not to mention it.

fpagnoux · 2017-06-09T11:27:22Z

tests/core/test_variable.py

+    assert simulation.calculate('end_attribute__formulas__different_names', month) == 300
+
+
+def test_clone__end_attribute__formulas__different_names():


I'm putting this as an optional feedback as we need to move on, but unit tests means than these tests should test one behaviour. This way, if a test does not pass, you know what is broken.

This tests is basically just inspecting the whole structure of the tax benefit system. There is no connection between cloning and checking that column_by_name is indeed storing what it is supposed to store.

In the future, please try to make your tests more focused.

It's ok do add all sorts of assertions in the code when developing, if it can help you understand the complicated and lopsided structure of OpenFisca.

But keep in mind that every line of code added to master, including tests, will have to be maintained and understood by OpenFisca developers.

Also, we want to change deeply the architecture of core. Over-testing it will have a cost : if our tests tend to check more than they should do, every time we will change something (for instance renamingIntCol), dozens of tests that have nothing to do with what we are doing will break.

It's was misleading of me to speak about unit tests. It started like that but they are more validation/system tests.

I do not think it is wise to add system tests to OpenFisca in its current state, especially in a file that is already over 500 lines. Please don't rigidify stuff that nobody understands properly. It is welcome to add guarantees to new behaviour, but not very useful to check that we don't change a behaviour that might be flawed.

fpagnoux

Needs third party mediation

fpagnoux · 2017-06-12T12:03:31Z

openfisca_core/formulas.py

+    elif end is not None:
+        assert isinstance(end, str), 'Type error on {}. String expected. Found: {}'.format(name + '.end', type(end))
+        try:
+            get_datetime_date(end)


@MattiSG,

We need another opinion to cut short this discussion.
We're inside the new_filled_column function, which builds an OpenFisca column (=variable) based on what has been coded by the legislation writer (class rsa(Variable)...).

The function get_datetime_date parses a 'YYYY-MM-DD' string and returns the equivalent datetime.date.

The suggested implementation here is to:

First call the get_datetime_date function once when building the column, just to check if it returns an error. We do not save the result.

Then, a little further, call it for each dated formula to check that its start date is consistent with the variable end date.

Finally, we also call it in the find_function method, whose job is to find the right dated function to use in a calculation. This method is called each time the variable is calculated.

I think there is no reason to parse a string 1 + k + n times.
(wherek = numbers of dated formulas, n = numbers of times the variable is calculated). I'm therefore in favour of parsing this string once and for all when we build the variable.

@sandcha thinks otherwise, and I'll let her expose her arguments.

@MattiSG wdyt ?

I suggest to declare an end attribute of date type to Formula class. There is no desagreement about not parsing a string multiple times.

Ok, great 🙂 . Tell me if you need help for this implementation!

I've been told you reached an agreement orally, glad to have been a good plastic duck 😄

I'm obviously in favour of limiting performance impact, if there is no good reason to multiply this parsing then let's not do it — duh.

I would like to bring your attention to the fact that it is more than time to ship, though. Too much energy and time is going into something that is not a priority. If it is too hard to reach a consensus on the implementation we'll have to abandon that story and learn from it. This is becoming too much of a drain on delivering value.
I'm sure you can get this done quickly. Please don't hesitate to take shortcuts, the quality of this code is already much above average. Better done than perfect.

fpagnoux

Ok, rebasing and merging

Checks interactions between start/stop_date attributes and decorated functions (DatedFormula). Removes last DatedVariable reference.

Catch expected AssertionError Correct syntax on variable calls

Split all tests into test_call/test_dates. Vectorize dummy functions output. Update tests so that they all pass > Represents variable dates management contract before #371 changes.

Keep only Variable class Minor correction for tests after DatedVariable/Variable merge

And move cache management to Holder

And correct and test Formula clone

And correct columns.is_input_variable Update tests accordingly

And names and comments cleaning

fpagnoux · 2017-06-23T13:00:27Z

Migration guide

The following instructions can help you adapt your OpenFisca country package to the new version of core.

Replacing variables `stop_date` attribute by `end`

Goal: Replacing attributes like stop_date = date(2014, 12, 31) by end = '2014-12-31'

Throughout your code, replace using regular expressions:

stop_date = date\((\d{4}), (\d{1}), (\d{1})\) by end = '\1-0\2-0\3'
stop_date = date\((\d{4}), (\d{2}), (\d{1})\) by end = '\1-\2-0\3'
stop_date = date\((\d{4}), (\d{1}), (\d{2})\) by end = '\1-0\2-\3'
stop_date = date\((\d{4}), (\d{2}), (\d{2})\) by end = '\1-\2-\3'

Manually removing or migrating formula's stop dates

The concept of a formula stop doesn't exist anymore.

You can identify them using the regular expression , stop = date\(\d{4}, \d{1,2}, \d{1,2}\).
For each of them though, a manual treatment is needed.

If the stop of the formula is just the day preceding the start of another formula, then this stop is redundant. The identified block can be removed.

For instance, the stop date is redundant in the following case:

@dated_function(start = date(1990, 1, 1), stop = date(2006, 12, 31))
def formula_1(...)
@dated_function(start = date(2007, 1, 1))
def formula_1(...)

If not, the stop of the formula probably indicates a global end date for the variable. In this case, manually transpose the end date as a end attribute of the variable.

Migrating `@dated_function(...)`

The formulas stop date must have been removed of migrated before starting this step

The goal is to replace code bits such as

    @dated_function(start = date(1990, 1, 1))
    def function_x(...):

by

    def function_1990_01_01(...):

Throughout your code, replace using regular expressions:

Be careful, spaces at the start of the line do matter

    @dated_function\(start = date\((\d{4}), (\d{2}), (\d{2})\)\)
    def (.)*\(

by

    def formula_\1_\2_\3(

and

    @dated_function\(start = date\((\d{4}), (\d{1}), (\d{2})\)\)
    def (.)*\(

by

    def formula_\1_0\2_\3(

and


    @dated_function\(start = date\((\d{4}), (\d{2}), (\d{1})\)\)
    def (.)*\(

by

    def formula_\1_\2_0\3(

and

    @dated_function\(start = date\((\d{4}), (\d{1}), (\d{1})\)\)
    def (.)*\(

by

    def formula_\1_0\2_0\3(

Renaming `function` by `formula`

Finally, throughout your code, replace using regular expressions def function( by def formula(

Manual final check

Automated research and replace can save you a significant amount of time, but a quick final human check is still recommended to make sure you didn't do anything wrong 🙂 .

sandcha requested a review from fpagnoux June 1, 2017 08:20

MattiSG changed the title ~~Dated variable #371~~ Merge Variables and DatedVariables Jun 1, 2017

fpagnoux requested changes Jun 1, 2017

View reviewed changes

fpagnoux requested changes Jun 7, 2017

View reviewed changes

sandcha requested a review from fpagnoux June 8, 2017 20:42

fpagnoux reviewed Jun 9, 2017

View reviewed changes

fpagnoux requested changes Jun 9, 2017

View reviewed changes

sandcha requested a review from fpagnoux June 9, 2017 17:44

fpagnoux requested changes Jun 12, 2017

View reviewed changes

sandcha requested a review from fpagnoux June 12, 2017 16:16

fpagnoux approved these changes Jun 12, 2017

View reviewed changes

sandcha and others added 13 commits June 12, 2017 20:17

Merge Variable and DatedVariable #371

d5bad19

Removes blanks for flake8.

2e0f638

Corrects DatedFormula evolution impact on web-api.

d7c1a63

Dated attributes vs dated functions.

1738aed

Checks interactions between start/stop_date attributes and decorated functions (DatedFormula). Removes last DatedVariable reference.

Extract tests on dated variables.

7c6177c

Improve test cases

f1ca38b

Catch expected AssertionError Correct syntax on variable calls

Update variables tests

b35c25b

Split all tests into test_call/test_dates. Vectorize dummy functions output. Update tests so that they all pass > Represents variable dates management contract before #371 changes.

Remove AbstractVariable class

8ccee82

Keep only Variable class Minor correction for tests after DatedVariable/Variable merge

Update DatedFormula computation steps

1abb0c5

And move cache management to Holder

Remove stop_instant from DatedFormula

53cf2ec

Pass API tests

46cf423

Remove Variable start_date attribute

c0704f3

Extract start instant from formula name

5fa4fd8

sandcha and others added 22 commits June 12, 2017 20:18

Lighten tests on Variable dates

5fad0c3

Adapt to openfisca_coutry_template module

b370459

Raise assert error when end < formula start date

6d81518

Minor change (remove extra_params)

fc716e7

Improve error message

337a213

Add Changelog

2a9087e

Put check in a nose test

802a1fd

Improve check_error_at_add_variable

25ec55e

Update test

68c8a93

Accept any input string in extract_formula_date_from_name

09c8956

And correct and test Formula clone

Correct regexp for dated formulas names

e49ee7d

And correct columns.is_input_variable Update tests accordingly

Add test_before_add__input_variable

d6fb1a5

And names and comments cleaning

Remove unnecessary None assertion on formula

199eb9b

Update full 13.0.0 changelog description

0a8590f

Ignore W503 outdated flake error

a2d33d9

Extract tests from test_variable

3df22fc

Correct get_datetime_date signature

d1e33df

Add s to test_variable file name

afca4ef

Extract constants for formula name

e2d3907

Parse end attribute in variables

7849f4f

Upgrade dependencies

7fcc097

Make github regexp more robust

c6c227e

fpagnoux force-pushed the dated-variable-#371 branch from 7438073 to c6c227e Compare June 12, 2017 18:20

fpagnoux merged commit 52f10d3 into master Jun 12, 2017

fpagnoux deleted the dated-variable-#371 branch June 12, 2017 18:25

This was referenced Jun 22, 2017

Upgrade Openfisca-Core to 14 betagouv/openfisca-cd93#4

Merged

Upgrade extension to Openfisca-Core 14 openfisca/openfisca-rennesmetropole#3

Merged

Upgrade extension to Openfisca-Core 14 openfisca/openfisca-paris#15

Merged

benjello mentioned this pull request Jun 18, 2019

Corrige la taxation des revenus des valeurs mobilières openfisca/openfisca-france#1343

Merged

		add_variable_catch_assertion(tax_benefit_system, variable__deprecated_start_date, 'Deprecated "start_date" attribute in definition of variable')


		# 371 - end, no function


		# end, one function without date

		class dated_attribute__one_simple_formula(Variable):

		end = '1989-12-31'


		check_error_at_add_variable(tax_benefit_system, variable__deprecated_start_date, 'Deprecated "start_date" attribute in definition of variable')

		formula_name_prefix + formula_name_separator + '%Y_%m_%d').date()


		def get_datetime_date(src_name, date_str):

		assert simulation.calculate('end_attribute__formulas__different_names', month) == 300


		def test_clone__end_attribute__formulas__different_names():

Merge Variables and DatedVariables #522

Merge Variables and DatedVariables #522

Conversation

sandcha commented Jun 1, 2017 • edited by MattiSG Loading

Breaking changes

New features

Technical changes

fpagnoux commented Jun 1, 2017

fpagnoux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux Jun 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux Jun 9, 2017 • edited Loading

Choose a reason for hiding this comment

fpagnoux Jun 9, 2017 • edited Loading

Choose a reason for hiding this comment

fpagnoux Jun 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux Jun 9, 2017 • edited Loading

Choose a reason for hiding this comment

fpagnoux left a comment

Choose a reason for hiding this comment

sandcha commented Jun 1, 2017 •

edited by MattiSG

Loading

fpagnoux left a comment •

edited

Loading

fpagnoux Jun 9, 2017 •

edited

Loading

fpagnoux Jun 9, 2017 •

edited

Loading

fpagnoux Jun 9, 2017 •

edited

Loading

fpagnoux Jun 9, 2017 •

edited

Loading

fpagnoux Jun 9, 2017 •

edited

Loading

fpagnoux Jun 9, 2017 •

edited

Loading