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

Fixed: Refactor scripts #91, Adds logging #85 and Cleans code and comments #86 by Putting it in a Shared Library/Module #96

Merged
merged 46 commits into from
Apr 2, 2024

Conversation

IamMQaisar
Copy link
Contributor

@IamMQaisar IamMQaisar commented Mar 17, 2024

Fixes

Description

The shared code including Logging from various scripts has been moved to a shared library named quantify.py.

  1. This Fix maintains all the comments and reorganization issues mentioned in Making Code and Comments consistent in codebase for Better Readability and Understanding #86
  2. This Fix adds a Feature of logging to the codebase as mentioned in [Feature] Integrate Python Logging into Codebase for Improved Management #85
  3. This Fix adds a Feature of Shared library/module in the codebase as mentioned in [Feature] refactor scripts to put shared code in a shared library #91

Technical details

  1. To Handle PYTHONPATH, sys.path.append has been used.
  2. logger is used in a shared module that logs Date ,time , module_name from where error generated and error msg.
  3. PATH_WORK_DIR exclusively handled as the sharing module quantify.py would have only shared it's own Path and not individual....Hard Coding the directory name would have negated the overall usage of getting the path from OS.PATH .

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the default branch of the repository (main or master).
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added or updated tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no
    visible errors.

Developer Certificate of Origin

For the purposes of this DCO, "license" is equivalent to "license or public domain dedication," and "open source license" is equivalent to "open content license or public domain dedication."

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@IamMQaisar IamMQaisar requested review from a team as code owners March 17, 2024 19:49
@IamMQaisar IamMQaisar removed the request for review from a team March 17, 2024 19:49
@IamMQaisar
Copy link
Contributor Author

I eventually cllosed my own 2 pull requests my self and merged that into one....so that it will not be some randomly modification again and again happening in all the pull requests, as first if loggers were added in all files and then again @TimidRobot had to review another pull and got them removed when a shared module is made then it will be a total waste of his time to go through same thing and get it change in other pull.

In that case did I copied myself and did plagiarism? 🤔No, I am just making life of the maintainer easy that He won't have to do it again and again. Same in my case and on reminding about the readme.md, I also did added that too. Which I eventually have added 2 lines myself but just went to sleep and did it on waking up now.

@IamMQaisar
Copy link
Contributor Author

Yes, I saw your pull, right, and yes I saw your added readme.md ...... My approach of adding logger is totally different than yours and to be mentioned that is adding readme.md doesn't necessarily states that I've copied your code or file. Kindly see the links as that is also different from yours.
It's a readme file, how's it that it's plagiarized?

I'm not saying that your code was plagiarized, I know that is original. I'm saying that your updates to README were the exact same as mine, and the sentence has the exact same words that I put in mine: "Built-in Python logging module to implement a flexible logging system across shared modules." and this is the link I also used: "[logging]: https://docs.python.org/3/howto/logging.html"

This was added to your PR after I submitted my PR, so I'm saying those lines were taken off of mine. Again, I'm not saying that your code was plagiarized, just wanted to clear up any confusion there!

Screenshot 2024-03-18 at 1 06 09 AM

Okay then let me change it for you, I just saw your pull and added my own readme.md too !

@naishasinha
Copy link
Member

Okay then let me change it for you, I just saw your pull and added my own readme.md too !

Thank you, I appreciate it!

@IamMQaisar
Copy link
Contributor Author

IamMQaisar commented Mar 18, 2024

Updated the readme.md , eventually it's not that I meant to do any copy, and I didn't even had the idea of plagiarism being considered on GitHub means it's all PUBLIC....Also, I go through all pulls to take ideas and update myself for learning purposes and when I encounter mistakes in there, I help those Pull Request makers too. Like yesterday in #92 , I just intend to help others contributors and maintainers and nothing else. Hope you understand !

@naishasinha
Copy link
Member

No worries, thank you for changing it!

@IamMQaisar IamMQaisar changed the title Fixed: Refactor scripts #90, Adds logging #85 and cleans code and comments #86 by putting it in a shared library Fixed: Refactor scripts #91, Adds logging #85 and Cleans code and comments #86 by Putting it in a Shared Library/Module Mar 18, 2024
@IamMQaisar
Copy link
Contributor Author

Hi @TimidRobot ,
Have you reviewed the pull and does it require any changes?

@TimidRobot
Copy link
Member

This size/scope of this PR makes it difficult to review.

Please resolve conflicts.

@IamMQaisar
Copy link
Contributor Author

IamMQaisar commented Mar 27, 2024

Hi @TimidRobot ,
I know, I've tried to resolve 3 PRs here but more likely it's actually better in sense the code won't be changing on each pull....., I will resolve the conflicts if there are any, just let me know of them.
is the wikipedia_scratcher.py having any conflict as it has been recently merged as new code into the main repo?

@IamMQaisar
Copy link
Contributor Author

IamMQaisar commented Mar 27, 2024

wikipedia_scratcher.py updated to the latest merge and other merge conflicts are also resolved. Let me know of any other changes if required.

Copy link
Member

@TimidRobot TimidRobot left a comment

Choose a reason for hiding this comment

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

please see requested changes and also update the other files accordingly

README.md Outdated Show resolved Hide resolved
deviantart/deviantart_scratcher.py Outdated Show resolved Hide resolved
deviantart/deviantart_scratcher.py Outdated Show resolved Hide resolved
analyze/data_analysis.py Outdated Show resolved Hide resolved
__init__.py Outdated Show resolved Hide resolved
deviantart/deviantart_scratcher.py Outdated Show resolved Hide resolved
deviantart/deviantart_scratcher.py Outdated Show resolved Hide resolved
analyze/data_analysis.py Outdated Show resolved Hide resolved
analyze/data_analysis.py Outdated Show resolved Hide resolved
analyze/data_analysis.py Outdated Show resolved Hide resolved
Copy link
Member

@TimidRobot TimidRobot left a comment

Choose a reason for hiding this comment

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

good work! thank you for seeing the PR through to completion

@TimidRobot TimidRobot merged commit 8412423 into creativecommons:main Apr 2, 2024
1 check passed
IamMQaisar added a commit to IamMQaisar/quantifying that referenced this pull request Apr 2, 2024
Merge pull request creativecommons#96 from IamMQaisar/shared_module
@TimidRobot TimidRobot self-assigned this Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants