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

removed old method to generate xml files. Added template method #4

Merged
merged 23 commits into from
Mar 18, 2024

Conversation

JJFlorian
Copy link
Contributor

No description provided.

@JJFlorian JJFlorian requested a review from reinout March 14, 2024 11:51
return rendered_xml

except Exception as e:
traceback.print_exc()
Copy link
Member

Choose a reason for hiding this comment

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

logger.exception() gebruiken (dan komt het ook in de logfile terecht)

Comment on lines +188 to +190
raise XMLGenerationError(
"De aangeleverde combinatie van request type en registratie type is niet mogelijk. Als de combinatie in de BRO wel mogelijk is, vraag dan deze combinatie aan bij Nelen & Schuurmans."
) from e
Copy link
Member

Choose a reason for hiding this comment

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

Als een error voorkomt, heeft het dan specifiek met deze foutmelding te maken?

@@ -0,0 +1,16 @@
<registrationRequest xmlns="http://www.broservices.nl/xsd/isgmn/1.0"
Copy link
Member

Choose a reason for hiding this comment

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

Ik zou de file gewoon .xml noemen?

@@ -0,0 +1,27 @@
<registrationRequest xmlns="http://www.broservices.nl/xsd/isgmn/1.0"
Copy link
Member

Choose a reason for hiding this comment

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

Ook hier weer .xml extensie gebruiken.

Comment on lines 17 to 18
</startDateMonitoring>{% for measuringpoint in sourcedocs_data.measuringPoints %}
<measuringPoint>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
</startDateMonitoring>{% for measuringpoint in sourcedocs_data.measuringPoints %}
<measuringPoint>
</startDateMonitoring>
{% for measuringpoint in sourcedocs_data.measuringPoints %}
<measuringPoint>

Zo'n "for" is het duidelijkst op z'n eigen regel.
Eventueel na de % spaties toevoegen om 'em duidelijker ergens bij te laten horen.


def test_xml_generator2():
"""Tests a non existing combination of registration_type and request type."""
with pytest.raises(delivery.XMLGenerationError):
Copy link
Member

Choose a reason for hiding this comment

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

Netjes om dit ook te testen. Dat gaat later als het project groter wordt enorm helpen.

Comment on lines +39 to +44
@pytest.fixture
def gmn(organisation):
return gmn_models.GMN(
data_owner=organisation,
bro_id="GMN123456789",
)
Copy link
Member

Choose a reason for hiding this comment

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

Voor het geval er veel fixtures zijn of voor het geval je ze in andere testfiles ook wilt gebruiken: je kan ze in een conftest.py verzamelen, dat wordt door pytest automagisch opgepikt.

https://docs.pytest.org/en/7.1.x/reference/fixtures.html#conftest-py-sharing-fixtures-across-multiple-files

Comment on lines +8 to +10
@pytest.fixture
def organisation():
return api_models.Organisation(name="Nieuwegein")
Copy link
Member

Choose a reason for hiding this comment

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

Goeie om hier een fixture voor te gebruiken. Die dingen maken het leven een stuk makkelijker :-)

Comment on lines 14 to 15
networks:
- backend
Copy link
Member

Choose a reason for hiding this comment

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

Het hele networks stuk mag er normaliter geloof ik uit. Standaard krijg je een default netwerk.
Aan de andere kant: laat nu maar staan, want in sommige cornercases heb ik het wel eens toe moeten voegen :-)

Comment on lines 21 to 22
ports:
- "6379:6379"
Copy link
Member

Choose a reason for hiding this comment

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

Deze is alleen nodig als je er vanaf buiten (=je laptop) tegenaan wil kletsen. Voor gebruik binnen de dockers zelf is het niet nodig.

@JJFlorian JJFlorian merged commit 1817de6 into main Mar 18, 2024
1 check passed
@JJFlorian JJFlorian deleted the template_setup branch March 22, 2024 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants