Skip to content

Fix issues with lint console output #6064

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

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

Conversation

ewels
Copy link
Member

@ewels ewels commented May 9, 2025

Noticed that nextflow lint is a little too aggressive and can start eating back on the terminal from before its own launch command.. 😬

Before this PR:

CleanShot 2025-05-10 at 00 26 45

After this PR:

CleanShot 2025-05-10 at 00 27 39

@ewels ewels requested a review from bentsherman May 9, 2025 22:28
Copy link

netlify bot commented May 9, 2025

Deploy Preview for nextflow-docs-staging canceled.

Name Link
🔨 Latest commit a905b83
🔍 Latest deploy log https://app.netlify.com/sites/nextflow-docs-staging/deploys/6821f00e631b2e0008a1ba18

@ewels ewels force-pushed the lint-aggressive-chomp branch from e4365f5 to 8d59b4f Compare May 9, 2025 22:28
@bentsherman bentsherman changed the title Remove one of the line chomps from the lint command Fix issues with lint console output May 9, 2025
@@ -90,7 +90,7 @@ class StandardErrorListener implements ErrorListener {

@Override
void onError(SyntaxException error, String filename, SourceUnit source) {
term.bold().a(filename).reset()
term.bold().a("error: ").a(filename).reset()
Copy link
Member

Choose a reason for hiding this comment

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

Let me know if you like how this looks and I'll merge. I wonder if it would be better to put the error/warning label after the location

Copy link
Member Author

Choose a reason for hiding this comment

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

Haven't checked, but yeah I was thinking after the file name - like a prefix to the error message:

Error: Something bad happened.

Maybe with some colour too? Need to look at it for that though.

Copy link
Member

Choose a reason for hiding this comment

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

Updated. Let me know how it looks

@ewels
Copy link
Member Author

ewels commented May 12, 2025

I think that it'd be improved with:

  • Prefixing the entire line - that way they all align and it's easy to scan up and down to see which are errors and which are warnings
  • More colour to better differentiate them, also helps to see the borders between each message.

I've pushed updates to that effect - I've somewhat controversially gone for a background colour which really makes it stand out:

CleanShot 2025-05-12 at 14 28 02@2x

In concise mode:

CleanShot 2025-05-12 at 14 30 59@2x

Is that too much? I can revert it to the same text but just regular Red / Yellow font colour instead of background colour if you prefer.

@ewels ewels force-pushed the lint-aggressive-chomp branch from c4cafe7 to bd22701 Compare May 12, 2025 12:32
@pditommaso
Copy link
Member

would be nicer to keep it align (also using abbreviation e.g. warn )

Signed-off-by: Phil Ewels <[email protected]>
@ewels
Copy link
Member Author

ewels commented May 12, 2025

Done 👍🏻 Had to pad Warn with spaces to get alignment but I think it's ok:

CleanShot 2025-05-12 at 14 56 03@2x

@pditommaso
Copy link
Member

Cool, tempted to make .2 to show this :D

@ewels
Copy link
Member Author

ewels commented May 12, 2025

Wondering if we should have a hanging block-colour to connect this to the context below 🤔 2 secs - will see if I can whip it up and send a screenshot

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.

3 participants