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

Added/Fixed Typing for all files in vunit.ui package #961

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

g0t00
Copy link

@g0t00 g0t00 commented Sep 28, 2023

No description provided.

vunit/test/bench.py Outdated Show resolved Hide resolved
from vunit.design_unit import DesignUnit

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
from vunit.design_unit import DesignUnit
from vunit.design_unit import DesignUnit

Not sure if any linting rules are applied, but this seems to make more sense either way.

Copy link
Author

Choose a reason for hiding this comment

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

I do not really catch what your saying. But I did not find clear style re: import statements.
Also after doing all the changes I realized that vunit uses relative imports for most/all internal imports.
I personally do not like relative imports in python, but for consistencies sake I would change. If somebody from the core team can pitch in...

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for being unclear, the point was that if there is an empty line, it should probably not happen between the vunit imports.

Now

from typing...
from vunit...

from vunit...

Better

from typing...

from vunit...
from vunit...

(Tools like isort would probably have done something like the latter anyway.)

Copy link
Author

Choose a reason for hiding this comment

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

Ah yes, thank you. Totally agreed.

(normally I just let isort handle imports)

@oscargus
Copy link
Contributor

oscargus commented Oct 6, 2023

Typing always helps! Some minor comments (I have no powers to approve or merge or whatever, but for what it is worth).

@umarcor
Copy link
Member

umarcor commented Mar 11, 2024

After rebasing on top of branch master, the following errors are produced in CI:

Linting

https://github.com/VUnit/vunit/actions/runs/8236613480/job/22523448088?pr=961#step:5:84

 ************* Module vunit.sim_if.__init__
vunit/sim_if/__init__.py:189:0: R0022: Useless option value for 'disable', 'no-self-use' was moved to an optional extension, see https://pylint.readthedocs.io/en/latest/whatsnew/2/2.14/summary.html#removed-checkers. (useless-option-value)
************* Module vunit.ui.preprocessor
vunit/ui/preprocessor.py:25:0: R0022: Useless option value for 'disable', 'no-self-use' was moved to an optional extension, see https://pylint.readthedocs.io/en/latest/whatsnew/2/2.14/summary.html#removed-checkers. (useless-option-value)

Building sdist

https://github.com/VUnit/vunit/actions/runs/8236613480/job/22523449340?pr=961#step:5:43

   File "/home/runner/work/vunit/vunit/vunit/configuration.py", line 192, in <module>
    class ConfigurationVisitor(object):
  File "/home/runner/work/vunit/vunit/vunit/configuration.py", line 200, in ConfigurationVisitor
    def get_configuration_dicts(self) -> List[OrderedDict[Any, Configuration]]:
TypeError: 'type' object is not subscriptable

Furthermore:

Ref #601.

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

Successfully merging this pull request may close these issues.

4 participants