Skip to content

Conversation

lenardt-gerhardts
Copy link
Contributor

@lenardt-gerhardts lenardt-gerhardts commented Sep 11, 2025

I have implemented the logger component based on the Idea #985, with the small change of renaming "output" to "message".

It is not documented yet, since i am still trying to wrap my head around the documentation, but i wanted to know what you think, before committing any further.

Copy link
Collaborator

@lovasoa lovasoa left a comment

Choose a reason for hiding this comment

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

Good, I'm in line with the general approach ! I made a few comments below.

src/render.rs Outdated
if let Some(value) = object_map.get(LOG_MESSAGE_KEY) {
log::log!(target: LOG_TARGET, log_level, "{value}");
} else {
return Err(anyhow::anyhow!("no message was defined for log component"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the log message should explicitly state where it comes from.

Something like :

log message from test.sql, statement 6: Hello World

You can use current_statement, and the request_context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I now added source_path from ParsedSqlFile to RequestContext. i had to change the visibility to pub instead of pub(crate) though. Did you have a different way in mind when telling me to use request_context?

@lovasoa
Copy link
Collaborator

lovasoa commented Sep 11, 2025

Good; I'll refactor to avoid the duplication and cloning between RequestInfo and RequestContext, but this is out of scope for this pr.

I think all that is missing before we can merge is

  • documentation (see for reference examples/official-site/sqlpage/migrations/06_debug.sql)
  • a test (maybe just a tests/sql_test_files/it_works_log.sql)

@lovasoa
Copy link
Collaborator

lovasoa commented Sep 11, 2025

Another thing that would be nice is for the log component to work while in header context. Currently logging something switches to html rendering, which means you can't log anything before the shell.

@lenardt-gerhardts
Copy link
Contributor Author

where can i define or search for a icon the component should get?

@lovasoa
Copy link
Collaborator

lovasoa commented Sep 11, 2025

https://tabler.io/icons

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