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

Initial library iteration #3

Merged
merged 41 commits into from
Dec 20, 2021
Merged

Initial library iteration #3

merged 41 commits into from
Dec 20, 2021

Conversation

Meldiron
Copy link
Contributor

No description provided.

LICENSE.md Outdated Show resolved Hide resolved
Copy link
Contributor

@christyjacob4 christyjacob4 left a comment

Choose a reason for hiding this comment

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

Please address the comments

composer.json Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Logging/Issue.php Outdated Show resolved Hide resolved
src/Logging/Issue.php Outdated Show resolved Hide resolved
src/Logging/Issue.php Outdated Show resolved Hide resolved
src/Logging/Issue/User.php Outdated Show resolved Hide resolved
src/Logging/Logging.php Outdated Show resolved Hide resolved
@Meldiron Meldiron changed the title Test Initial library iteration Nov 11, 2021
tests/LoggingTest.php Outdated Show resolved Hide resolved
@eldadfux
Copy link
Member

@Meldiron I had an idea. Could be nice to open multiple issues for the community for adding new adapters for other logging platforms. We should probably also have a simple contribution guide.

@Meldiron
Copy link
Contributor Author

@christyjacob4 Let's rename repository to logger

@Meldiron
Copy link
Contributor Author

Prepared GH issue for a provider. Please review it before I create an issue for each provider. #4

@kodumbeats
Copy link

Does this library need to support PHP7.4? Can we run with PHP8 or 8.1?

@Meldiron
Copy link
Contributor Author

Meldiron commented Dec 3, 2021

Does this library need to support PHP7.4? Can we run with PHP8 or 8.1?

@eldadfux Your decision probably. I would guess code can work without problem on both. With v8, there might be features to make code cleaner.

@eldadfux
Copy link
Member

eldadfux commented Dec 3, 2021

@Meldiron We can stick to 8.0 and 8.1.

@Meldiron
Copy link
Contributor Author

Meldiron commented Dec 5, 2021

@kodumbeats @eldadfux Upgraded requirements to PHP 8.0 in commit db66b8c. Please let me know if that is all or I missed something.

Does that even exist?? 😅
src/Logger/Log.php Outdated Show resolved Hide resolved
src/Logger/Log.php Outdated Show resolved Hide resolved
src/Logger/Log.php Outdated Show resolved Hide resolved
src/Logger/Log.php Outdated Show resolved Hide resolved
Copy link
Contributor

@christyjacob4 christyjacob4 left a comment

Choose a reason for hiding this comment

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

Just a minor style comment.
Please make sure all the opening braces in all the functions start in new lines. Rest looks great! Amazing job 🥳

@Meldiron Meldiron requested a review from eldadfux December 14, 2021 10:03
@eldadfux
Copy link
Member

@kodumbeats @eldadfux Upgraded requirements to PHP 8.0 in commit db66b8c. Please let me know if that is all or I missed something.

You should be good 👍

Copy link
Member

@eldadfux eldadfux left a comment

Choose a reason for hiding this comment

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

This is beautiful work. The codebase looks much cleaner with the new changes, and implementation of a new adapter looks much easier. I added a few comments, most of them regarding naming changes.

README.md Outdated Show resolved Hide resolved
src/Logger/Adapter.php Outdated Show resolved Hide resolved
src/Logger/Adapter/Raygun.php Outdated Show resolved Hide resolved
src/Logger/Adapter/Raygun.php Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@eldadfux eldadfux merged commit a7d626e into main Dec 20, 2021
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.

4 participants