snet-cli icon indicating copy to clipboard operation
snet-cli copied to clipboard

Suggestions for a cleaner code

Open arturgontijo opened this issue 6 years ago • 11 comments

I'd like to suggest the followings changes (all minor):

  1. Using F-String (since snet-cli has python 3.6 as requirement)
  2. Remove redundant parentheses. https://github.com/singnet/snet-cli/blob/master/snet_cli/init.py#L24
  3. Remove white spaces on variable assignment. https://github.com/singnet/snet-cli/blob/master/snet_cli/commands.py#L276
  4. Remove white spaces on kwargs. https://github.com/singnet/snet-cli/blob/master/snet_cli/commands.py#L174
  5. Add/Remove newlines after/before function/class declarations accordingly to PEP8.

I can do this with #251.

arturgontijo avatar May 14 '19 19:05 arturgontijo

  • I'm a little bit warring about F-string. From the first glance f-string might be a security risk (because inside {} you can execute arbitrary function).
  • I'm strongly against 3. white spaces on variable assignment makes code more clear.
# like this is more clear
x = a + b

It is especially true in case of alignment

x   = b
x1  = c + b
x33 = d + c
  • Again I'm strongly against "4". It makes code more clear.

astroseger avatar May 15 '19 08:05 astroseger

There is PEP8 "Style Guide for Python Code" https://www.python.org/dev/peps/pep-0008. For instance section https://www.python.org/dev/peps/pep-0008/#id26 is about spaces in statements. I would propose stick to it as it seems to be industry standard. We could add style checker as a CI step to ensure that style is correct.

vsbogd avatar May 15 '19 08:05 vsbogd

@astroseger I think this security risk exists with format() too, right?

About the points 2, 3, 4 and 5: I suggested them because some IDEs, like mine (PyCharm), highlights them and I think sooner or later users will come with these suggestions too.

@vsbogd I like to follow PEP8 as much as "I can", but somethings that it recommends (eg max 79 chars per line) can be "very polemic".

arturgontijo avatar May 15 '19 10:05 arturgontijo

@arturgontijo, sorry I didn't get you already mentioned PEP8, and probably what you mean is sticking to it except max line length :-) You point (3) just sounds ambiguous but actually it is about two spaces on the right of the = sign instead of one and not about removing spaces at all.

vsbogd avatar May 15 '19 10:05 vsbogd

  • About f-strings... yes it seems you cannot pass arbitrary string from outside and than "execute" it in f-string... I would still vote for allowing old %-format just because I like it, but I will not strongly argue for it...
  • if we do not fully follow PEP8, why not add two exceptions: "79 chars" and "alignment"

I do see the reason why they do not allow alignment in PEP8 (because it is difficult to formalize it), but forbidding alignment will make code much less readable, so I'm against it.

for example:

        state["current_nonce"]          = int.from_bytes(response.current_nonce,         byteorder='big')
        state["current_signed_amount"]  = int.from_bytes(response.current_signed_amount, byteorder='big')

        state["current_nonce"] = int.from_bytes(response.current_nonce, byteorder='big')
        state["current_signed_amount"] = int.from_bytes(response.current_signed_amount, byteorder='big')

astroseger avatar May 15 '19 10:05 astroseger

@vsbogd yeah the point 3 is about the extra white space.

I agree to follow PEP8 100%, but like I said this can cause a lot of infinite discussion (cause this tend to become a personal style discussion).

I don't like the whites spaces on variable assignment but I'm good with them. So to make it a standard for future contribution I think we need to use it everywhere: For example: https://github.com/singnet/snet-cli/blob/master/snet_cli/commands.py#L287-L289 https://github.com/singnet/snet-cli/blob/master/snet_cli/mpe_channel_command.py#L200-L208

I can try to change these parts (align with white spaces) but I've never coded like this, so maybe the outcome will not be what you're expecting =/.

arturgontijo avatar May 15 '19 11:05 arturgontijo

@arturgontijo yes... it is why PEP8 forbids alignment with white-spaces... Actually another problem is variable renaming...

After thinking about it, I would suggest two alternative options.

  1. allow alignment as an option without forcing it everywhere.
  2. Fully follow PEP8 as was suggested by @arturgontijo (probably without 79-character limitation).

I would go with 1, but if someone would like to fully stick to PEP8 I wouldn't argue...

astroseger avatar May 15 '19 12:05 astroseger

I'll try to achieve 2 using some automation and if the result is good I open a PR.

arturgontijo avatar May 16 '19 11:05 arturgontijo

@arturgontijo @astroseger @vsbogd what do you think about tools like https://github.com/marketplace/restyled-io? Was thinking of setting up such bots to enforcing linting (where applicable) and to enforce code style guidelines. As we open up code to the community we should have tooling to retain coding standards.

raamb avatar Jun 21 '19 09:06 raamb

@raamb, yes, looks good

vsbogd avatar Jun 21 '19 09:06 vsbogd

@raamb, let's do this!

arturgontijo avatar Jun 21 '19 16:06 arturgontijo