Conversation
rehanhaider
commented
May 7, 2021
rehanhaider
commented
- Arranged the app in a more modular fashion
- Changed the config management to the python standard ConfigParser library
- Added an option through "settings.ini" to play Surah Baqarah on Fridays
…guration instead of the custom method being used currently 2. Replaced the mergeArgs() with getConfig() method to a. First check if mandatory arguments, lat, lon, method are stored in settings.ini b. If not, look at the args provided during startup c. Set the args and return it 3. Changed job commands to make it fully compatible with python3 4. Improved the formatting of output to be more descriptive 5. Added a method to play surah baqarah on fridays as 8 am 6. Updated README
|
I just merged a PR that was created prior to yours which has created some conflicts. Could you please resolve conflicts and re-submit the PR? Thanks! |
Accepted both changes in .gitignore
|
Have resolved the conflicts now. |
Sourcery Starbot ⭐ refactored justgoodin/adhan
| @@ -0,0 +1,21 @@ | |||
| # MIT License | |||
|
|
|||
| **Copyright (c) 2021 justgoodin** | |||
There was a problem hiding this comment.
what's is justgoodin? and why are we copyrighting this to it?
There was a problem hiding this comment.
Generated it using GitHub's license generator to make it fully permissive. Somehow they added this, I didn't notice.
| parser.add_argument('--lat', type=float, dest='lat', | ||
| help='Latitude of the location, for example 30.345621') | ||
| parser.add_argument('--lng', type=float, dest='lng', | ||
| parser.add_argument('--lon', type=float, dest='lon', |
There was a problem hiding this comment.
How about we call it long? I believe lat/long is the usually the standard.
Also, could you update run it for the first time section in README to match this? I think it still says lng
| } | ||
|
|
||
|
|
||
| # Setup Surah Baqarah on Fridays |
There was a problem hiding this comment.
Playing Surah Barqah at 8am on Friday seems like a super specific requirement. I'm not sure how many people, esp the ones in Western countries, would benefit from this feature.
I would like to keep this project simple and only make it an Azaan player. I would suggest creating a separate script for playing surahs? People who wish to play surahs can install it separately and run both scripts on their raspberry pis. Thoughts?
There was a problem hiding this comment.
It is disabled by default, but makes sense. It will unnecessarily increase the size of the download as well.
|
Happy to merge most of your changes but not convinced that we should add the Surah Baqrah feature. See my comments inline. Thanks! |
|
In my view you should reject/close the PR.
All the changes are very specific to what I need and I agree it probably doesn't go with your vision.
…On 16 Jul 2021, 1:37 AM +0530, Abrar Chaudhry ***@***.***>, wrote:
Happy to merge most of your changes but not convinced that we should add the Surah Baqrah feature. See my comments inline. Thanks!
—You are receiving this because you authored the thread.Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Are you sure brother? You did a lot of work and I'd hate to not incorporate it. If you want to take out the Surah Baqrah part, I'm more than happy to merge the rest of it. |
|
Nah it's OK, my OCD made me organise this a bit better, but I didn't change any functionality so continue working seamlessly for everyone. I'll continue using my own fork with customisations I made as per my need. |
3 participants
