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

Add .log support to Monaco preview handler #21362

Merged
merged 4 commits into from
Nov 14, 2022
Merged

Add .log support to Monaco preview handler #21362

merged 4 commits into from
Nov 14, 2022

Conversation

Eagle3386
Copy link
Contributor

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Adds .log file extension to monaco_languages.json as supported plaintext file.

Validation Steps Performed

Rather small addition, hence added the extension locally (via Registry) & immediately noticed succeeding file preview.

@GitMensch
Copy link

What do you think about the additional log names ".err" and (possibly more conflicting) ".out"?

@Eagle3386
Copy link
Contributor Author

Missing feedback from @crutkas or @Aaron-Junker within the linked issue made me focus on .log.

In addition to that, your "possibly more conflicting" hint fueled my believe that error and/or general output should rather be "redirected" into error.log & output.log respectively even more.

Ultimately, I'd suggest using .log as a "catch-all" logging file type & a file's actual name, i. e. everything left to the last ., for stating what content (type) such log files hold.

@GitMensch
Copy link

I see. Still suggest to add ".err" ;-)

Copy link
Collaborator

@Aaron-Junker Aaron-Junker left a comment

Choose a reason for hiding this comment

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

monaco_languages.json gets suto-generated. You have to add another handler like in

registerAdditionalLanguage("xmlExt", [".xsd", ".wsdl", ".xslt"], "xml", monaco)

Copy link
Collaborator

@stefansjfw stefansjfw left a comment

Choose a reason for hiding this comment

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

@Aaron-Junker I'm leaving this to you :) Good catch

@Eagle3386
Copy link
Contributor Author

Eagle3386 commented Nov 1, 2022

@Aaron-Junker:

monaco_languages.json gets suto-generated. You have to add another handler like in

registerAdditionalLanguage("xmlExt", [".xsd", ".wsdl", ".xslt"], "xml", monaco)

Just to get you right:

  1. I have to undo my changes inside monaco_languages.json?
  2. Register a new handler despite there's no such thing as syntax highlighting for .log files & plaintext .txt files are already handled without such a custom handler?
  3. There's no simple "register .log as another plaintext file type" change possible?

@Eagle3386
Copy link
Contributor Author

@stefansjfw:

@Aaron-Junker I'm leaving this to you :) Good catch

May I ask you to remove your change request then? Because once I solved Aaron's request, yours seems to still block my PR. Or am I getting something wrong here?

@Aaron-Junker
Copy link
Collaborator

@Aaron-Junker:

monaco_languages.json gets suto-generated. You have to add another handler like in

registerAdditionalLanguage("xmlExt", [".xsd", ".wsdl", ".xslt"], "xml", monaco)

Just to get you right:

  1. I have to undo my changes inside monaco_languages.json?
  2. Register a new handler despite there's no such thing as syntax highlighting for .log files & plaintext .txt files are already handled without such a custom handler?
  3. There's no simple "register .log as another plaintext file type" change possible?
  1. Yes.
  2. Yes. And then you have to regenerate monaco_languages.json like described here: https://github.com/microsoft/PowerToys/blob/main/doc/devdocs/modules/powerpreview/monaco/readme.md

@Eagle3386
Copy link
Contributor Author

Alright, doin' some rocket science then - back in a day or two, keep your fingers crossed! 🥳

@stefansjfw stefansjfw self-requested a review November 1, 2022 18:03
@Eagle3386
Copy link
Contributor Author

@Aaron-Junker Done with the changes, but IMHO this could've been done way easier & faster by just stating:

  1. Add ,{"id":"logExt","extensions":[".log"]} to monaco_languages.json.
  2. Add registerAdditionalLanguage("logExt", [".log"], "log", monaco) to monacoSpecialLanguages.js, replace a wrong <tab> char with 4 spaces, remove a doubled trailing empty line in there, too & you're done.

This way, I'd have been much quicker getting this done & hence the preview handler supporting log files, too.
Anyway, hope everything is well now - if not, please tell me right away & I'll fix it ASAP. :)

@Eagle3386 Eagle3386 requested review from Aaron-Junker and removed request for stefansjfw November 1, 2022 19:08
@Eagle3386
Copy link
Contributor Author

@Aaron-Junker The run failed due to some NuGet plugin error on Azure DevOps side, i. e. beyond my control - would you mind, getting the appropriate person to fix that & tell me how to re-trigger a run?

@Aaron-Junker
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Eagle3386
Copy link
Contributor Author

@Aaron-Junker Run completed, checks passed, LGTY?

@stefansjfw stefansjfw self-requested a review November 9, 2022 08:56
Copy link
Collaborator

@Aaron-Junker Aaron-Junker left a comment

Choose a reason for hiding this comment

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

I'm currently not able to test it, becasue of time reasons. But for me it looks good. And I don't see a reason why it shouldn't work

@Eagle3386
Copy link
Contributor Author

@Aaron-Junker I'm leaving this to you :) Good catch

@stefansjfw since Aaron approved it & you stated that, you'll be leaving this to him, would you mind adding your approval, too?
Because GitHub tells me it won't merge until you approved, too…

@Eagle3386 Eagle3386 requested review from Aaron-Junker and removed request for stefansjfw and Aaron-Junker November 9, 2022 18:21
@Eagle3386 Eagle3386 requested review from stefansjfw and Aaron-Junker and removed request for stefansjfw November 9, 2022 18:21
Copy link
Contributor Author

@Eagle3386 Eagle3386 left a comment

Choose a reason for hiding this comment

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

@stefansjfw AFAICT, this is the commit you need to either approve or remove yourself from the list of reviewers for this very checkin.

@Eagle3386 Eagle3386 requested review from stefansjfw and removed request for Aaron-Junker November 9, 2022 18:24
Copy link
Collaborator

@stefansjfw stefansjfw left a comment

Choose a reason for hiding this comment

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

tested. looks good

@jaimecbernardo jaimecbernardo merged commit 4897c44 into microsoft:main Nov 14, 2022
@jaimecbernardo
Copy link
Collaborator

Thanks a lot for the contribution!

@Eagle3386 Eagle3386 deleted the patch-1 branch November 18, 2022 08:26
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.

5 participants