-
Notifications
You must be signed in to change notification settings - Fork 143
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
Implement vif extensions #166
Implement vif extensions #166
Conversation
2adc49b
to
17a7328
Compare
I don't know why it has been closed. I tried to push a new revision (a simple rebase). |
0ebfdd6
to
fff71f8
Compare
fff71f8
to
6c1e202
Compare
Pushed a new version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks unrelated to the pull request description. Why is it not mentioned there?
@@ -241,7 +241,7 @@ | |||
<DataRecord id="30"> | |||
<Function>Instantaneous value</Function> | |||
<StorageNumber>0</StorageNumber> | |||
<Unit>Unrecognized VIF extension: 0x60</Unit> | |||
<Unit>Reset counter</Unit> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to have updates to the test frames as a separate commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am used to have automatic tests being executed, so I did update the code and the tests at the same time.
Do you insist on splitting it into separate commits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean #165 enforces this?
I didn't come a final decision about this, but i'm concerned about possible corner cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, #165 would enforce that you have to update the tests when you change the code.
Can you describe a corner case?
I'm quite new to all this, there could be some magic string that you can put in the commit message in order to skip running the CI jobs.
My concern is that I don't want to try to do a change, and find out that the tests are not in sync with the code. ("It was broken when I got it")
EDIT: Having automatic tests is current best practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not against automatic tests. I'm only concerned that the tests apply on commit level and not on pull request level.
Unfortunately i don't have a specific corner case in mind. On a negative side this could lead to the situation that contributors tend to put everything in a single commit.
So at least for this pull request, i'm fine with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how github works. I think that github run tests on the latest commit (on a branch or pull request).
Once you are actually uesd to it, it seems natural, when I do see a code update, I expect to find corresponding updates in the tests. It is very seldom that we do change the xml-format an will have to update every test-file.
@lategoodbye I would like input on my questions? I would like this to be done and ready ASAP. |
Please point me to an question which isn't answered here. |
Sorry, but I think I reloaded and did not see any replys. |
7bafe4d
to
1621759
Compare
1621759
to
eda6c89
Compare
(cherry picked from commit 5b26e62)
(cherry picked from commit 67ea2d3)
Add all codes for the vif extensions.