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.

Sign up for GitHub

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

Jump to bottom

Add search condition for getting articles #2

Merged
merged 3 commits into from
Aug 6, 2019
Merged

Add search condition for getting articles #2

merged 3 commits into from
Aug 6, 2019

Conversation

JosephT5566
Copy link
Contributor

Hi
我試著增加了一些features，
再麻煩你有空的時候幫忙review一下看看是否合適~~
Thanks

Changes

  1. 在getArticles()中新增search條件(by push, author or title)
  2. 新增相對應的unit test
  3. 新增launch.json來實現mocha debug

Note

  1. 目前中文的關鍵字title搜尋無法成功，不清楚是不是編碼的問題，還需要trace一下
  2. mocha debug的部分用起來感覺還是不太穩定，不知道是不是遺漏了什麼QQ

@JosephT5566
[Merge]
114940d 
1. Add debug option for mocha
2. Add search feature and correspond unit test.
kevinptt0323
kevinptt0323 reviewed Aug 5, 2019
View reviewed changes
Copy link
Owner

@kevinptt0323 kevinptt0323 left a comment

Choose a reason for hiding this comment

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

首先非常感謝你的 PR 😃

Mocha Test 的部分我也常常跑到壞掉，不知道是不是跟 PTT 連線機制有關係。

這個 PR 看起來不支援多重搜尋（例如 /JOJO /心得 Z20 之類的），也許我們之後可以再新增。

vscode 我自己是沒有在用，請問可以改成呼叫 npm run test 嗎？timeout 等參數你可以修改 package.json。

src/sites/ptt/bot.js Show resolved Hide resolved
src/sites/ptt/bot.js Outdated Show resolved Hide resolved
src/sites/ptt/bot.js Outdated Show resolved Hide resolved
src/sites/ptt/bot.js Outdated Show resolved Hide resolved
test/articles.js Outdated Show resolved Hide resolved
@JosephT5566
Copy link
Contributor Author

不確定欸，不知道是不是因為太頻繁登入之類的關係，有時候我卡住我就先把確定pass的給skip掉就能繼續執行了。
對欸！我沒想到多重搜尋這一層，或許後許再來新增~
好~謝謝你的建議，後續再試試看一些參數設定，不過debug功能不太靈光，我現在也只是拿他來跑單一測試檔(或許launch.json就不要更新上來?)。

kevinptt0323
kevinptt0323 reviewed Aug 5, 2019
View reviewed changes
src/sites/ptt/bot.js Outdated Show resolved Hide resolved
src/sites/ptt/bot.js Show resolved Hide resolved
kevinptt0323
kevinptt0323 requested changes Aug 5, 2019
View reviewed changes
Copy link
Owner

@kevinptt0323 kevinptt0323 left a comment

Choose a reason for hiding this comment

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

偶爾突然測不過我暫時也沒解法。
launch.json 看你要不要這次先拿掉，之後有更好的 config 可以再發 PR 之類的。
單一搜尋完成我就 approve 了。

@JosephT5566
[Modify] Modify issues and codes
41b3314 
1. execute resetSearchCondition() by searchCondition.init()
2. change searchCondition.Type to searchCondition.type
3. remove launch.json
4. modify isSearchConditionSet() method
@JosephT5566
Copy link
Contributor Author

先把launch.json拿掉好了，之後有更好的作法的話再提出~

@kevinptt0323 kevinptt0323 merged commit fae3879 into kevinptt0323:master Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinptt0323 kevinptt0323 kevinptt0323 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JosephT5566 @kevinptt0323