Skip to content

Conversation

bonjourmauko
Copy link
Member

Technical changes

@bonjourmauko bonjourmauko added the kind:fix Bugs are defects and failure demand. label Mar 16, 2021
@bonjourmauko bonjourmauko requested review from a team March 16, 2021 13:53

def test_variable_formula_content():
formula_code = "def formula(person, period, parameters):\n return person('salary', period) * parameters(period).taxes.income_tax_rate\n"
formula_code = "def formula(person, period, parameters):\n \"\"\"\n Income tax.\n\n The formula to compute the income tax for a given person at a given period\n \"\"\"\n return person(\"salary\", period) * parameters(period).taxes.income_tax_rate\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

could it be easier to read as:

Suggested change
formula_code = "def formula(person, period, parameters):\n \"\"\"\n Income tax.\n\n The formula to compute the income tax for a given person at a given period\n \"\"\"\n return person(\"salary\", period) * parameters(period).taxes.income_tax_rate\n"
formula_code = """
def formula(person, period, parameters):
\"\"\"
Income tax.
The formula to compute the income tax for a given person at a given period
\"\"\"
return person("salary", period) * parameters(period).taxes.income_tax_rate
"""

Copy link
Contributor

Choose a reason for hiding this comment

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

^^^ Suggestion only, I'll hit approve button

@MattiSG MattiSG mentioned this pull request Mar 17, 2021
2 tasks
@MattiSG
Copy link
Member

MattiSG commented Mar 22, 2021

It would be great if this landed soon, all tests fail until this one is merged 🙂 For example, #982 is blocked because of that.

@MattiSG
Copy link
Member

MattiSG commented Mar 22, 2021

@maukoquiroga unless you'd prefer to finalise this, I intend to merge it today to unblock #982 🙂

@MattiSG
Copy link
Member

MattiSG commented Mar 22, 2021

This test could be made much more reliable by asserting the presence of a subset of the text rather than asserting full string equality.

@MattiSG MattiSG mentioned this pull request Mar 22, 2021
@MattiSG MattiSG merged commit f769804 into master Mar 23, 2021
@MattiSG MattiSG deleted the fix-doc-test-api branch March 23, 2021 08:57
@bonjourmauko
Copy link
Member Author

Hi @MattiSG, thanks for the proposed PR, accepted it, way better !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:fix Bugs are defects and failure demand.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Failing Web API tests

3 participants