-
Notifications
You must be signed in to change notification settings - Fork 263
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
Use more f-strings for formatting strings #747
Conversation
FTR, @NanooooK confirmed that acceptance tests are passing with ModelSim. Two of the examples are failing for legit reasons, unrelated to this PR. |
I apologize but I didn't run the tests correctly (wrong branch). I need to re-run them. Now I got the following
|
@umarcor Riviera-PRO works with exception of the |
@umarcor Active-HDL has the same result as Riviera-PRO. |
@umarcor For me with ModelSim using
or
|
@NanooooK, can you please run the acceptance tests again? |
@LarsAsplund this was a conscious decision. The used syntax is valid VHDL 2008, so it should be supported by simulators which are compliant. However, there are some simulators which don't support the syntax, but we don't know certainly which. Also, we don't know if the feature is missing because of the license tier. Therefore, having this test work in CI but fail when running acceptance tests with other simulators is made to raise awareness of the capabilities that VHDL 2008 (PSL) provides and which we are not using in the community. Nonetheless, I agree that it can be painful, and from VUnit's perspective it might not be desirable that new users find this error and they are forced to understand the motivation. I am OK with disabling it for simulators other than GHDL, but I would like have some accurate handling of the case. For instance, we can mark it as Therefore, if you are ok, I will mark it as XFAIL when simulators other than GHDL are used, and I will open an issue for tracking an improvement of the reasons for each sim. |
Two tests failed but as you said they are out of scope for this PR (verilog_ams, and the assert in the axis_vcs_example) |
@NanooooK I think that verilog_ams requires a good enough license. What ModelSim are you using? |
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 reviewed all the files now.
@LarsAsplund I am using ModelSim DE. The error for AMS test is the following: |
@NanooooK Yeah, it might not be sufficient for AMS. Siemens has Questa-ADMS which might be the only product supporting this. |
This PR complements #743. The remaining f-string related pylint issues are fixed. However, I could not test these, because I don't have access to all those simulators. Therefore, I would be grateful if other users could test this branch and report any issue.