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

Clean code #1081

Merged
merged 16 commits into from
May 8, 2019
Merged

Clean code #1081

merged 16 commits into from
May 8, 2019

Conversation

chenjiandongx
Copy link
Member

本次 PR 内容,

  1. 整理代码
  2. 新增示例

@codecov-io
Copy link

codecov-io commented May 6, 2019

Codecov Report

Merging #1081 into dev will increase coverage by 0.49%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #1081      +/-   ##
==========================================
+ Coverage   80.92%   81.42%   +0.49%     
==========================================
  Files          82       83       +1     
  Lines        1809     1787      -22     
==========================================
- Hits         1464     1455       -9     
+ Misses        345      332      -13
Impacted Files Coverage Δ
pyecharts/__init__.py 100% <ø> (ø) ⬆️
pyecharts/charts/__init__.py 100% <ø> (ø) ⬆️
pyecharts/charts/basic_charts/bar.py 100% <ø> (ø) ⬆️
pyecharts/_version.py 100% <ø> (ø) ⬆️
pyecharts/charts/basic_charts/bmap.py 100% <100%> (ø) ⬆️
test/test_series_options.py 100% <100%> (ø) ⬆️
test/test_bmap.py 100% <100%> (ø) ⬆️
test/test_bar.py 100% <100%> (ø) ⬆️
test/test_sunburst.py 100% <100%> (ø) ⬆️
pyecharts/options/global_options.py 83.92% <100%> (ø) ⬆️
... and 9 more

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 07f1c78...43817fc. Read the comment docs.

{% endif %}
{% for js in c.js_functions.items %}
Copy link
Member

Choose a reason for hiding this comment

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

这个 js 放在 setOption 后面会影响 echarts.registerMap 这个函数

#!/usr/bin/env python
# coding=utf-8
import asyncio
from aiohttp import TCPConnector, ClientSession

import pyecharts.options as opts
from pyecharts.charts import Map

"""
Gallery 使用 pyecharts 1.0.0
参考地址: https://echarts.baidu.com/examples/editor.html?c=map-HK

目前无法实现的功能:

1、不知道小红点咋去掉,无伤大雅
"""


async def get_json_data(url: str) -> dict:
    async with ClientSession(connector=TCPConnector(ssl=False)) as session:
        async with session.get(url=url) as response:
            return await response.json()


# 获取官方的数据
data = asyncio.run(
    get_json_data(url="https://echarts.baidu.com/examples/data/asset/geo/HK.json")
)

map_data = [
    ["中西区", 20057.34],
    ["湾仔", 15477.48],
    ["东区", 31686.1],
    ["南区", 6992.6],
    ["油尖旺", 44045.49],
    ["深水埗", 40689.64],
    ["九龙城", 37659.78],
    ["黄大仙", 45180.97],
    ["观塘", 55204.26],
    ["葵青", 21900.9],
    ["荃湾", 4918.26],
    ["屯门", 5881.84],
    ["元朗", 4178.01],
    ["北区", 2227.92],
    ["大埔", 2180.98],
    ["沙田", 9172.94],
    ["西贡", 3368],
    ["离岛", 806.98],
]


name_map_data = {
    "Central and Western": "中西区",
    "Eastern": "东区",
    "Islands": "离岛",
    "Kowloon City": "九龙城",
    "Kwai Tsing": "葵青",
    "Kwun Tong": "观塘",
    "North": "北区",
    "Sai Kung": "西贡",
    "Sha Tin": "沙田",
    "Sham Shui Po": "深水埗",
    "Southern": "南区",
    "Tai Po": "大埔",
    "Tsuen Wan": "荃湾",
    "Tuen Mun": "屯门",
    "Wan Chai": "湾仔",
    "Wong Tai Sin": "黄大仙",
    "Yau Tsim Mong": "油尖旺",
    "Yuen Long": "元朗",
}

(
    Map(init_opts=opts.InitOpts(width="1400px", height="800px"))
    .add_js_funcs("echarts.registerMap('HK', {});".format(data))
    .add(
        series_name="香港18区人口密度",
        maptype="HK",
        data_pair=map_data,
        name_map=name_map_data,
    )
    .set_global_opts(
        title_opts=opts.TitleOpts(
            title="香港18区人口密度 (2011)",
            subtitle="人口密度数据来自Wikipedia",
            subtitle_link="http://zh.wikipedia.org/wiki/%E9%A6%99%E6%B8%AF%E8%A1%8C%E6%94%BF%E5%8D%80%E5%8A%83"
            "#cite_note-12 ",
        ),
        tooltip_opts=opts.TooltipOpts(
            trigger="item", formatter="{b}<br/>{c} (p / km2)"
        ),
        visualmap_opts=opts.VisualMapOpts(
            min_=800,
            max_=50000,
            range_text=["High", "Low"],
            is_calculable=True,
            range_color=["lightskyblue", "yellow", "orangered"],
        ),
    )
    .render("population_density_of_HongKong.html")
)

Copy link
Member

@chfw chfw May 7, 2019

Choose a reason for hiding this comment

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

是应该放在前面

@@ -18,7 +19,7 @@ def __init__(self, init_opts: Union[opts.InitOpts, dict] = opts.InitOpts()):
self.js_dependencies.add("bmap")
self._is_geo_chart = True
self._coordinate_system: Optional[str] = "bmap"
self.js_bmap_functions: utils.OrderedSet = utils.OrderedSet()
self.js_functions: utils.OrderedSet = utils.OrderedSet()
Copy link
Member

Choose a reason for hiding this comment

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

this reveal a problem in our unit test :). should have caught it!

@@ -178,15 +180,6 @@ def add_schema(
emphasis_itemstyle_opts: Union[opts.ItemStyleOpts, dict, None] = None,
emphasis_label_opts: Union[opts.LabelOpts, dict, None] = None,
):
if isinstance(label_opts, opts.LabelOpts):
Copy link
Member

Choose a reason for hiding this comment

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

💯

from unittest.mock import patch

from nose.tools import eq_
Copy link
Member

Choose a reason for hiding this comment

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

useless?

Copy link
Member Author

Choose a reason for hiding this comment

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

没看懂 什么意思?

Copy link
Member

Choose a reason for hiding this comment

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

在这个文件,用到了吗?

Copy link
Member Author

Choose a reason for hiding this comment

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

已经被移除

c = (
BMap()
.add_schema(
baidu_ak="Uf1rIjuIVVXxDwEy0iEU0tApwdoqGeGn",
Copy link
Member

Choose a reason for hiding this comment

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

this api key should not be inserted here! because it is real and belongs to you.

Copy link
Member

Choose a reason for hiding this comment

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

get this from an evironment variable

set API_KEY='.d.d.d.d.d'

in code,

import os
...
baidu_ak=os.environ['API_KEY']
..

Copy link
Member Author

Choose a reason for hiding this comment

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

这个建议不错

@chenjiandongx
Copy link
Member Author

@chfw @sunhailin-Leo 请 Rivew PR,有问题的话请提出。这个 PR 积累的内容已经有点多了,不想再扩展了,需要合并

title=self.options.get("title", opts.TitleOpts().opts) + title
)
if isinstance(title, opts.TitleOpts):
title = title.opts
Copy link
Member

Choose a reason for hiding this comment

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

TitleOpts inherits from BaseOpts?

Copy link
Member Author

Choose a reason for hiding this comment

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

这个情况特殊 只能在这里判断

from ..globals import BMapType


class BasicOpts:
pass
__slots__ = ("opts",)
Copy link
Member

Choose a reason for hiding this comment

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

hohoho, sexy slots!

Copy link
Member Author

Choose a reason for hiding this comment

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

concise, but powerful

@@ -1,5 +1,7 @@
from unittest.mock import patch

from nose.tools import assert_in, assert_not_in, eq_
Copy link
Member

Choose a reason for hiding this comment

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

learnt assert_in and assert_not_in, I did not know their existence before!

from nose.tools import eq_

from pyecharts import options as opts
from pyecharts.charts import Bar, Timeline


class TestTimeLine:
class TestTimeLine(unittest.TestCase):
Copy link
Member

Choose a reason for hiding this comment

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

这个好像,有没有无所谓。unittest.TestCase 有一个函数 assetMultilineEqual ,倒是很有用。

Copy link
Member

Choose a reason for hiding this comment

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

nose 会好好测试。

Copy link
Member Author

Choose a reason for hiding this comment

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

加上去会好一点

Copy link
Member

@chfw chfw left a comment

Choose a reason for hiding this comment

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

再改改。

Copy link
Member

@chfw chfw left a comment

Choose a reason for hiding this comment

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

Good to go!

@chfw
Copy link
Member

chfw commented May 7, 2019

@sunhailin-Leo , 还有意见吗?

@sunhailin-Leo
Copy link
Member

@sunhailin-Leo , 还有意见吗?

👌

@chenjiandongx chenjiandongx merged commit b8aad7d into dev May 8, 2019
@chenjiandongx chenjiandongx deleted the clean-code branch May 8, 2019 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants