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

feat: logger #200

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

feat: logger #200

wants to merge 7 commits into from

Conversation

roman533
Copy link
Contributor

@roman533 roman533 commented Jun 7, 2023

PR description

Describe your changes in detail here

Created a composable for a logger with electron-log and tslog

Definition Of Done (DoD)

This PR can be squashed / merged if

  • a developer is assigned
  • the PR is NOT estimated
  • the PR is labeled
  • the PR is NOT assigned to the current sprint
  • a meaningful title has been set according to https://www.conventionalcommits.org/
  • the PR is described in detail
  • the PR links to an issue
  • the PR has been reviewed

fixes: #167
Add additional conditions here if necessary for this PR

BeierKevin
BeierKevin previously approved these changes Jun 7, 2023
Copy link
Contributor

@BeierKevin BeierKevin left a comment

Choose a reason for hiding this comment

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

I think this looks good and could work nicely when implemented into the existing codebase

Copy link
Member

@Claiyc Claiyc left a comment

Choose a reason for hiding this comment

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

Generally good implementation (code style wise), however does not work for me yet

electron/preload/index.ts Outdated Show resolved Hide resolved
electron/preload/index.ts Show resolved Hide resolved
/**
* Electron log levels enum declaration
*/
export enum ElectronLogLevel {
Copy link
Member

Choose a reason for hiding this comment

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

just updating the values for these enums to numbers (as in .env) may be a good solution

Copy link
Member

Choose a reason for hiding this comment

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

However you need to make sure that eg. loglevel INFO also includes everything WARN & ERROR (this only makes sense)

Copy link
Contributor

@BeierKevin BeierKevin Jun 7, 2023

Choose a reason for hiding this comment

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

Maybe we still need to discuss what we really want out of this

src/composables/useLogger/useLogger.ts Outdated Show resolved Hide resolved
src/composables/useLogger/useLogger.ts Outdated Show resolved Hide resolved
src/composables/useLogger/useLogger.ts Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

the logfile should be created in the root directory of the installation location (and not somewhere in appdata). Maybe in some subfolder logs/

Copy link
Member

Choose a reason for hiding this comment

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

This needs to be tested for an actually installed application as well as in dev environment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logger
3 participants