Cleanup of the qtgui for pep8

[fviard]

Hi @papapel , Looking at the Gui you provided, I think that you might still be new to python, so I hope that you will not mind my following recommandations:

For the GUI, the code is working good, but I see that you are not following the usual Python "codestyle", so I think that you may not be aware of it.

For Python, the codestyle is not very strongly enforced, but there is a standard one that is recommended by the Python project and that is used in almost all projects. It is called PEP-8 (because it is the number of the PEP specification defining it). By following it, you make the code more readable and easy to understand by other people.

The best resource I have found to give a summary of the rules is this page: https://pep8.org/

Also, there are 2 python command line tools that can be useful as they scan the code, and inform you of the line that are not conformant: pylint or pycodestyle

So, to come to the point of this issue: It would be nice if you could rework the code style of the gui and do a new PR with the changes so that it will be more conformant to pep-8.

For example, I see a lot of issues with long lines and missing spaces around "=" for assignements.

I could do it, but I don't have so much free time :-p and also, foremost, I think that it would be a good exercice for you if you want to gain in Python skills.

Thank you! Don't hesitate to tell me if you need help or have questions.

[papapel]

Hello @fviard!

Thank you for sharing me your tips. Actually, I am not new to Python but I am new to share code on Github. I don't know the good practices well. You did well to open this issue.

At work I am using Black. But I will use PEP-8 to share code with this project. Then I will rework the code style of the gui and do a new PR.

Thank you!

[fviard]

Great :+1: Btw, but it is just my personal opinion, but I dislike a lot Black. It is like the Go formater some guys imposed their (bad) taste to the code, and automatically without reflection. By that, I mean, when you do it manually, you think, and sometimes you change/break the rules or adapt them in some cases to ensure that it really look good.

[fviard]

So, as said in the PR, here are few additional general suggestions of improvements: 1) Not in a rigourous "black" way, i find it sometimes better to do it like this the parenthesis opening is not too far: Instead of:

self.readerParam = Parameter.create(
    name='params', type='group', children=readerSettingsParams)

One of these:

self.readerParam = Parameter.create(name='params', type='group',
                                    children=readerSettingsParams)
or
self.readerParam = Parameter.create(name='params',
                                    type='group',
                                    children=readerSettingsParams)

2) Dict alignement: Instead of:

readerSettingsParams = [
    {'name': 'time',
        "title": "Time (seconds to inventory)", 'type': 'float', 'value': 10},

Do that:

readerSettingsParams = [
    {'name': 'time', "title": "Time (seconds to inventory)",
     'type': 'float', 'value': 10},
    ...
]
(it's ok if not the same number of args per line, for each {} entry)
or 
readerSettingsParams = [
    {
        'name': 'time',
        "title": "Time (seconds to inventory)",
        'type': 'float',
        'value': 10
     },
    ...
]

3) when you use logging.error, .warning, .debug, there is simpler way than using the %: Instead of:

logging.error('First: %s, Second: %s" % (arg1, arg2))

You can do:

logging.error('First: %s, Second: %s", arg1, arg2)

(Just take care that it works for logging functions, but not print, or exception message and co ...)

4) Use string format instead of string concatenation when possible, as it simplify and the code will even be more performant: Instead of:

logger.info(
            str(tags) + " tag_filter_mask=<" +
            str(self.reader.llrp.config.tag_filter_mask) + ">"
        )

Do:

logger.info("%s tag_filter_mask=<%s>", str(tags),
                  str(self.reader.llrp.config.tag_filter_mask))

5) Last, but the best one: don't hesitate to use local temporary variables, it will do a simpler code that will also go faster!!! For example, in that case:

        self.window.connectionButton.setText("Connect")
        self.window.connectionButton.setChecked(False)
        self.window.connectionStatusCheckbox.setChecked(False)
        ...

You will use more than once self.window, so in that case, it is better to put it in a temp var like:

    window = self.window
    window.connectionButton.setText("Connect")
    window.connectionButton.setChecked(False)
    window.connectionStatusCheckbox.setChecked(False)

It will be faster, because each time you have the ".", python will have to do the lookup for the "window" variable inside the self object. Where otherwise, the "pointer reference" to the window subobject is already stored, so no lookup needed.

Same effect, even more interesting, you can use it for a dict or function in something like that:

            if tari is None:
                tari = self.readerParam.param("tari").value()
            if session is None:
                session = self.readerParam.param("session").value()
            if mode_identifier is None:
                mode_identifier = \
                    self.readerParam.param("mode_identifier").value()

transformed into

            r_param_func = self.readerParam.param
            if tari is None:
                tari = r_param_func("tari").value()
            if session is None:
                session = r_param_func("session").value()
            if mode_identifier is None:
                mode_identifier = r_param_func("mode_identifier").value()

In fact, let me add a last point: 6) Take care of the variable name case: For variables and function, use snake case (ex. reader_param). And use camel case, only for Class name (ex.: ReaderConfig).

So it gives things like that : reader_config = ReaderConfig(some_parameter=True)

[papapel]

Hello @fviard !

Sorry for this late response. I was not available. Thank you very much for giving me all this tips. It is very useful and I have used them for last months.

I will clean-up the qtgui for pep8 this summer, when I will have time.

Otherwise, With my team we decided to use black + pre-commit because we do not want waste our time with code formatting (and because sametmax as well). But I have no problem to use your rules to contribute to your project :)

[fviard]

No problem, and sorry for my own late response also :-p

For black I understand the logic, but if you allow me to share my opinion, I think the same as what is described in this blog: https://luminousmen.com/post/my-unpopular-opinion-about-black-code-formatter and what looks like to be also the opinion of the creator of Python: https://twitter.com/gvanrossum/status/1227252290580410368

As you will have probably seen with the other issue, I moved/reworked the gui to the following project: https://github.com/fviard/sllurp-gui I hope that you will find time to continue contribue to it there.

Btw, you can check that the email address you used for your commit are registered inside github, because commits do not look like to be associated with your github profile. Also, maybe you would like to have your real name associated with the credits and co in this project? I just had your username.

[papapel]

Thank you for sharing me this blog. I am interested to learn an other approach.

Thank you for creating the sllurp-gui project. You did a great work! It looks like really nice. I will check the TODO and select item when I will have time to contribute.

And here is my real name: Adrien Vialletelle (adrien.vialletelle@gmail.com)

[papapel]

I think you can close this issue :slightly_smiling_face: