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

Asks bids #171

Merged
merged 15 commits into from
Aug 8, 2022
Merged

Asks bids #171

merged 15 commits into from
Aug 8, 2022

Conversation

sfmqrb
Copy link
Collaborator

@sfmqrb sfmqrb commented May 15, 2022

در اینجا با رسیدگی به ادامه ایشوی #160 به دریافت دیتای لحظه ای order book یا همان ask-bid پرداختم که برای همه تیکرها به صورت یکجا دیتا را دریافت و در قالب یک خروچی csv و پانداس دیتا فریم خروجی میدهد

هنوز البته readme را آپدیت نکرده ام
منتظرم چند کار دیگر که کردم یکجا آپدیت کنم.

@lgtm-com
Copy link

lgtm-com bot commented May 15, 2022

This pull request introduces 2 alerts when merging b7bc2d1 into a9c6601 - view on LGTM.com

new alerts:

  • 2 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented May 28, 2022

This pull request introduces 3 alerts when merging 2afec08 into f65a313 - view on LGTM.com

new alerts:

  • 3 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented May 29, 2022

This pull request introduces 3 alerts when merging 74a5d5b into f65a313 - view on LGTM.com

new alerts:

  • 3 for Unused import

@sfmqrb
Copy link
Collaborator Author

sfmqrb commented May 29, 2022

اول تصمیم گرفته بودم فقط ask-bid در این pr اضافه شود ولی الان pr من شامل همه دیتاهای ارزشمند دیده بان بازاره. 😄
این تقریبا نهاییه. البته یه جاییش رو میشه ارتقای پرفورمنسی خوبی داد ولی شاید بعدا بیام سراغش یا یکی دیگه بره سرش.
به نظرم آماده مرچ شدنه.
@Glyphack

@Glyphack
Copy link
Owner

سلام @sfmqrb ببخشید من درگیر امتحانا بودم این رو تا فردا شب چک میکنم حتما مرسی

@sfmqrb
Copy link
Collaborator Author

sfmqrb commented May 30, 2022

سلام @sfmqrb ببخشید من درگیر امتحانا بودم این رو تا فردا شب چک میکنم حتما مرسی

اوکیه آقا عجله ای نیستش راحت باش

@@ -98,3 +137,33 @@ def get_aggregated_key_stats(base_path=None, to_csv=False)\
aggregated_key_stats_df.to_csv(path, index=False)

return aggregated_key_stats_df


def _get_key_stats(session):
Copy link
Owner

Choose a reason for hiding this comment

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

بنظرم این توابع رو نیاز نداری، چون الان فقط داره ریکوئست میزنه و دیتا رو برمیگردونه اگر همین یه خط کد ریکوئست رو ببریم توی تابع و همرو با همدیگه یه ترای کچ بزاریم راحت تره خوندنش.
اما کار دیگه‌ای که میشه کرد اینه که این تابع یه مقدار کار بیشتری بکنه، مثلا خود ریسپانس صفحه اطلاعات کلیدی خیلی خروجی راحتی نیست کار کردن باهاش میتونیم یه ذره اطلاعات صفحه رو پارس کنیم و در قالب یه dataclass برگردونیم اینطوری خیلی خروجی تابع خوانا میشه که چیا رو میده و هرجا که به اطلاعات آمار کلیدی نیاز باشه میشه همین تابع رو استفاده کرد.
بنظر من کار دوم رو انجام بدیم. و برای توابع پایینی هم انجام بده لطفا.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

بیبن اینکه دیتاکلاس بکنیم رو من باهات موافقم خوانا تر میشه.
ولی خیلی تغییرات توی کد زیاد میشه که تا الان دولوپ شده حتی از مرج های قبلی.
کلا من باهاشون مثل دیکشنری برخورد کردم و همه منطق روی این موضوع سواره.
اگر موافق باشی این رو به عنوان یه فیچر nice to have برای تمیز کردن کد پیشنهاد کنیم. ایشالا فرصت کردم خودم میرم سراغش.
به نظرم چون هم توی توابع مربوط به ساخته شدن دیکشنری ها داخل stats.py کامنت ها خوانایی رو ممکن میکنه و هم درباره key_stats توضیحات مفصلی داخل این ریدمی هست و اینکه کد هم کار میکنه :) فعلا از این تغییر بگذریم.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

این توابع رو هم میتونم داخل خود کد بیارم.
فعلا براشون جای جدا گذاشتم اگر بخواد منطق هندل کردن ریکوئست ها همه یه جا جمع بشه درباره stats ها.
مثلا برای هندل کردن ارور های عجیبی که ممکنه بعدا tsetmc بده و خوبه توی اکسپشن ها بعدا هندل بشه.
الان البته فقط جاش رو گذاشتم و اروری خیلی هندل نمیشه و فقط یه پیام چاپ میشه ولی بعدا میشه منطقش رو پیچیده تر کرد.
بازم اگر نظرت اینه که این توابع رو حذف کنم تو این مرحله بگو

Copy link
Owner

@Glyphack Glyphack left a comment

Choose a reason for hiding this comment

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

بازم ببخشید خیلی طول کشید نگاه کنم ولی دیگه هستم از امروز. کامنت‌هارو ببین اگر چیزی رو هم خواستی بیشتر دربارشون حرف بزنیم بهم بگو حتما میتونیم یه جلسه ست کنیم.

@sfmqrb
Copy link
Collaborator Author

sfmqrb commented Jul 10, 2022

بازم ببخشید خیلی طول کشید نگاه کنم ولی دیگه هستم از امروز. کامنت‌هارو ببین اگر چیزی رو هم خواستی بیشتر دربارشون حرف بزنیم بهم بگو حتما میتونیم یه جلسه ست کنیم.

سلام خوبی؟
اوکیه بابا من خودمم تا دیروز درگیر امتحانا بودم.
این هفته کامنتات رو بررسی می کنم ایشالا 😉

@sfmqrb
Copy link
Collaborator Author

sfmqrb commented Aug 7, 2022

اگر اوکی بود به نظرت تغییرات و مواردی که گفتم بگو قبل مرج فایل داکیومنتیشن پکیج رو با این فیچر های جدید یه آپدیت بکنم تا قابل استفاده باشه برای یوزر.
@Glyphack

@sfmqrb sfmqrb requested a review from Glyphack August 7, 2022 18:45
@Glyphack
Copy link
Owner

Glyphack commented Aug 7, 2022

سلام دستت درد نکنه فرد بعد از ظهر چک میکنم

@lgtm-com
Copy link

lgtm-com bot commented Aug 7, 2022

This pull request introduces 1 alert when merging 57a520d into 563c995 - view on LGTM.com

new alerts:

  • 1 for Unused import

@Glyphack
Copy link
Owner

Glyphack commented Aug 8, 2022

@sfmqrb همه چیز عالیه میتونی داکیومنت رو هم آپدیت کنی؟

@sfmqrb
Copy link
Collaborator Author

sfmqrb commented Aug 8, 2022

@sfmqrb sfmqrb closed this Aug 8, 2022
@sfmqrb
Copy link
Collaborator Author

sfmqrb commented Aug 8, 2022

آقا دستم خورد کلوز کردم
میتونی اوپن کنی؟
@Glyphack

@Glyphack Glyphack reopened this Aug 8, 2022
@sfmqrb
Copy link
Collaborator Author

sfmqrb commented Aug 8, 2022

@sfmqrb همه چیز عالیه میتونی داکیومنت رو هم آپدیت کنی؟

حله حتما

@sfmqrb
Copy link
Collaborator Author

sfmqrb commented Aug 8, 2022

حله @Glyphack
آماده مرجه

@Glyphack Glyphack merged commit 833d8a1 into Glyphack:master Aug 8, 2022
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.

2 participants