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

fix: remove unnecessary printlns #960

Merged
merged 1 commit into from
Feb 22, 2024
Merged

Conversation

amaanq
Copy link
Contributor

@amaanq amaanq commented Feb 21, 2024

It seems like it was probably for debugging, but should've been removed before #931 was merged.

Thanks!

Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

How about we turn them into warnings?

They are quite useful when debugging

@amaanq
Copy link
Contributor Author

amaanq commented Feb 21, 2024

The problem is in tree-sitter we use cc to build binary artifacts (parsers) at runtime here, so every single time a user makes a change in their parser this message pops up and is quite annoying. If it were a warning, would that be configurable in any way?

@NobodyXu
Copy link
Collaborator

If it were a warning, would that be configurable in any way?

yes, Build::cargo_warning can disable it.

@amaanq
Copy link
Contributor Author

amaanq commented Feb 21, 2024

perfect! should I just edit it to have an if cargo_output.warnings check for each case and print if so?

@NobodyXu
Copy link
Collaborator

You just need to use cargo_output: &CargoOutput,, it will handle it for you (you'll need to add it to params of wait_on_child)

@amaanq
Copy link
Contributor Author

amaanq commented Feb 21, 2024

fixed

src/command_helpers.rs Outdated Show resolved Hide resolved
src/command_helpers.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

Thank you!

@NobodyXu NobodyXu merged commit 3f59e09 into rust-lang:main Feb 22, 2024
21 checks passed
@amaanq
Copy link
Contributor Author

amaanq commented Feb 22, 2024

Thanks a ton for the quick reviewing!!

@amaanq
Copy link
Contributor Author

amaanq commented Feb 22, 2024

Just a small request, if we could have a minor release somewhat soon that'd be really nice for tree-sitter to also have a minor patch bump as well to get rid of this intrusive runtime logging 😁

@NobodyXu
Copy link
Collaborator

I can cut a new release in one week, given that we already got one release this week.

@amaanq
Copy link
Contributor Author

amaanq commented Feb 23, 2024

that works great, thank you

@NobodyXu
Copy link
Collaborator

Turns out that we need another logging level for such information.

@amaanq
Copy link
Contributor Author

amaanq commented Feb 24, 2024

I think that might be a good idea, to be honest I still sorta doubt the usefulness of logging what's running and the output status code, but I can see why it might be nice to have. Do you think a debug level better suits this?

@NobodyXu
Copy link
Collaborator

I suppose we just add another function debugging_print(bool) to disable it, specifically for your ise case.

It will be printed without cargo:warnings= just like how it was before this PR, but allows you to disable it to get back the old behavior.

@amaanq
Copy link
Contributor Author

amaanq commented Feb 24, 2024

Alright I've put up #972, I think that debug logging should be disabled by default - let's continue the conversation there

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