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

Add stock-tracker #6387

Merged
merged 1 commit into from
Sep 7, 2019
Merged

Add stock-tracker #6387

merged 1 commit into from
Sep 7, 2019

Conversation

beacoder
Copy link
Contributor

Brief summary of what the package does

stock-tracker enable users to track chinese stocks prices in emacs with json-api provided by netease company.

Direct link to the package repository

https://github.com/beacoder/stock-tracker

Your association with the package

I'm the maintainer.

Relevant communications with the upstream package maintainer

N/A

Checklist

Please confirm with x:

  • The package is released under a GPL-Compatible Free Software License.
  • [x ] I've read CONTRIBUTING.org
  • [ x] I've used the latest version of package-lint to check for packaging issues, and addressed its feedback
  • [ x] My elisp byte-compiles cleanly
  • [ x] M-x checkdoc is happy with my docstrings
  • [ x] I've built and installed the package using the instructions in CONTRIBUTING.org
  • I have confirmed some of these without doing them

@beacoder beacoder changed the title Create stock-tracker Add stock-tracker Aug 18, 2019
@alphapapa
Copy link
Contributor

Since the package is only for Chinese stocks using a certain proprietary API, the name should be descriptive. stock-tracker implies much more general usefulness.

@beacoder
Copy link
Contributor Author

@alphapapa currently it did only utilize a certain proprietary API which used for chinese stock market.
But in the future, I may add more open APIs to retrieve data from stocks which may come from other markets, so I think the name is appropriate.
thanks.

@riscy
Copy link
Member

riscy commented Aug 24, 2019

Just a quick check since these packages might be quite different, but any relationship (or possible opportunity for mutual benefit) with https://github.com/hagleitn/stock-ticker?

@beacoder
Copy link
Contributor Author

beacoder commented Aug 24, 2019

@riscy , nope, no relationship with this stock-ticker.
the stock-tracker is based on net-ease open api for tracking stocks listed in CHN and US.
in stock-tracker, there's a dedicated buffer showing stocks.
seems in stock-ticker, it just show stock information in mode-line.

@riscy
Copy link
Member

riscy commented Sep 1, 2019

Taking a closer look at this now.

Personally I'm okay with the name of the package (stock-ticker, after all, accesses Yahoo's API, and that appears to be US-only at a quick glance). More descriptive is almost always better, so I'd be thrilled if this package could be renamed netease-stocks or similar. But that's not a blocker.

However, I think it would be better if stock-ticker's package description mentioned it was US stocks -- and by that same token, I think it would be good if stock-tracker's description mentioned it was Chinese stocks. This will help people searching for packages know what they're installing.

To be equivocal equitable :) about this, I've raised an issue at the stock-ticker author's repository: hagleitn/stock-ticker#6

Some other miscellaneous advice I have:

  • stock-tracker.el#L238: It's safer to sharp-quote function names; use #'
  • stock-tracker.el#L263: It's safer to sharp-quote function names; use #'
  • stock-tracker.el#L384: It is beneficial to sharp-quote hooks; use #'
  • stock-tracker.el#L25: Prefer https over http (if possible)
  • stock-tracker.el#L71: Prefer https over http (this one seems like it might be a very good idea if possible)

Minor:

  • error-reponse -> error-response
  • moniter -> monitor

@riscy riscy added the awaiting-upstream Awaiting action from an upstream maintainer label Sep 1, 2019
@beacoder
Copy link
Contributor Author

beacoder commented Sep 1, 2019

Thanks for these great advices, I will take time to go over these one by one.
thanks a lot.

@beacoder
Copy link
Contributor Author

beacoder commented Sep 2, 2019

@riscy As I have updated in the https://github.com/beacoder/stock-tracker/blob/master/README.org
This package now support US stocks as well.
Please see the note region.
Thanks.

@beacoder
Copy link
Contributor Author

beacoder commented Sep 2, 2019

@riscy I have fixed all 7 issues you mentioned, thanks a again.

@riscy riscy removed the awaiting-upstream Awaiting action from an upstream maintainer label Sep 7, 2019
@riscy
Copy link
Member

riscy commented Sep 7, 2019

Great, thanks! :)

@riscy riscy merged commit 5e95930 into melpa:master Sep 7, 2019
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.

None yet

3 participants