Adressing proposed changes

[epifanio]

Summary:

Addressing changes proposed from PR review - GH-14

Related issue:

#17

Suggested reviewer(s):

@mortenwh
@ElodieFZ

Checklist:

• The headers of all files contain a reference to the repository license (i.e., "License: This file is part of py-mmd-tools, licensed under the Apache License 2.0 (https://www.apache.org/licenses/LICENSE-2.0)")
• The test coverage increased or remained the same as before
• Every function is accompanied with a test suite
• Tests are both positive (testing that the function work as intended with valid data) and negative (testing that the function behaves as expected with invalid data, e.g., that correct exceptions are thrown)
• Functions with optional arguments has separate tests for all options
• Examples are supported by doctests
• All tests are passing
• Documentation is complete and understandable

The checklist is based on the S-ENDA conventions and definition of done. The above points are not necessarily relevant to all contributions. In that case, please add a short explanation to help the reviewer.

[mortenwh]

Thanks. I have added inline comments for revision. It's a bit strange that the coverage report doesn't show up in the PR. I'm wondering if it is because you're pushing from a fork.. Could you check? We need to be able to see the coverage status and change in the PR conversation.

[mortenwh]

Also note that it is the reviewer who should tick off the checklist. I un-ticked the one about coverage now because it is not evident to the reviewer.

[epifanio]

Thanks. I have added inline comments for revision. It's a bit strange that the coverage report doesn't show up in the PR. I'm wondering if it is because you're pushing from a fork.. Could you check? We need to be able to see the coverage status and change in the PR conversation.

The fancy graph is enabled per-repository and doesn't work across forks. However, you can still see the coverage from the logs -> Show all checks -> Details -> Test and coverage -> scroll the log

[mortenwh]

What about this one? codecov/codecov-action#29

It seems it has been solved for forks. Can you have a look? I have to run to a meeting now, and the rest of the day is taken. I'll look at the rest when you're done with your updates.

[epifanio]

What about this one? codecov/codecov-action#29

It seems it has been solved for forks. Can you have a look? I have to run to a meeting now, and the rest of the day is taken. I'll look at the rest when you're done with your updates.

I applied most of the changes.
I checked the topic you pointed at, it is a long discussion and I have no experience to look into it - if you find examples or instructions I will be happy to follow them.

[epifanio]

Also note that it is the reviewer who should tick off the checklist. I un-ticked the one about coverage now because it is not evident to the reviewer.

I see - you are right - However, as you can see from the build logs, it was an objective and quantitative comparison between the coverage percentage from master and this branch.

[mortenwh]

There's a few things here which are still a bit unclear to me. I also made a few changes. I have merged that to master, and then we can take it gradually. Also, it seems we should have tests for script/xmlconverter.py as well.