numberoverzero / bottom

asyncio-based rfc2812-compliant IRC Client
http://bottom-docs.readthedocs.io
MIT License
74 stars 23 forks source link

add TOPIC parsing #46

Closed Yay295 closed 7 years ago

Yay295 commented 7 years ago

and unify the formatting

codecov[bot] commented 7 years ago

Codecov Report

Merging #46 into master will decrease coverage by 0.67%. The diff coverage is 85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
- Coverage   99.31%   98.63%   -0.68%     
==========================================
  Files           5        5              
  Lines         436      439       +3     
  Branches      119      120       +1     
==========================================
  Hits          433      433              
- Misses          2        4       +2     
- Partials        1        2       +1
Impacted Files Coverage Δ
bottom/unpack.py 97.98% <85%> (-2.02%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update bab0f3b...2b6cbc1. Read the comment docs.

numberoverzero commented 7 years ago

Can you split the reformatting changes and the functional changes into different PRs? It's a bit more work but I'd rather minimize the moving parts when reviewing new functionality.

I'm very glad for the consistency improvements though. This code is crufty. For example not only does this need an extra space, but the outer parens are redundant:

if(len(params) > 2):

should really be:

if len(params) > 2:
Yay295 commented 7 years ago

Sure. One question, I think

kwargs["message"] = params[1] if len(params) > 1 else ""

is cleaner, but the rest of the file uses

if len(params) > 1:
    kwargs["message"] = params[1]
else:
    kwargs["message"] = ""

Which one should I use?

Yay295 commented 7 years ago

new pull request is #48