search

no-stack-dub-sack / apexdox-vs-code

A lightweight VS Code extension that makes documenting your Salesforce Apex files an easy, integrated experience.
Other
49 stars 7 forks source link

H2 closing tag to H3 #44

Closed dschach closed 2 years ago

dschach commented 2 years ago

Simplified PR for fixing a single issue. I still cannot get tests to run on my machine - I'm getting an error about vscode not being found. This is probably because of the new split to two modules, but I did not do the split myself. I don't know why there's a new line in package.lock about tslint - anyway, that's been deprecated, but I included it anyway. And I removed pretty because I've been having trouble with pre tags in my projects, and I think that may solve them. It's not needed anyway, since anyone can run Prettier on the html pre-commit if they choose.

no-stack-dub-sack commented 2 years ago

Hey, thanks - as I mentioned in the other thread, I've been dealing with a bit of a family emergency lately so I may not be fully responsive again for the next several days. Will try to check in when I can.

Regarding Pretty, can you give an example of the issues you're having with pre tags? I'd like to definitively narrow down the issue as being caused by Pretty as overall I'm a fan of shipping this in a manner where the output docs are already readable. However if its def causing issues, as you say its not a big stretch for end-users to format themselves with a more modern tool.

dschach commented 2 years ago

Hi @no-stack-dub-sack Sorry about the family emergency - definitely understand.

I have also used Prettier, which is, well, opinionated. But I don't think my problem is related to that because:

I uninstalled

So all that's left in my project is 

├── auto-changelog@2.4.0
├── husky@7.0.4
├── lint-staged@12.3.7
└── prettier-plugin-apex@1.10.0

My homepage for ApexDox has code blocks wrapped in <pre><code> tags. If I run ApexDox, it takes the code in the pre/code blocks and removes all the line breaks. So something is breaking in the way that ApexDox is reading my sample code. The only part of ApexDox that changes html is Pretty.

When I run the page through the Prettier.io page, everything is fine. So the problem must be in either:

So there's nothing else interfering, and though Pretty says it doesn't touch pre/code blocks, it looks like it does. Pretty just wraps js-beautify, so...

Two options:

  1. Don't make anything pretty when the pages are created, but encourage people to use husky and lint-staged to make changes to the html (which is what I do).
  2. Skip Pretty and to go straight to js-beautify, as that allows creating our own options array (which maybe we can expose to the user, but I don't think we should).

Also, we should make the test for the homepage more complex and include sample inline code such as installation instructions and some Apex, just to prove how things are formatted.

I can try adding in js-beautify where Pretty was, but because I can't get any tests to run (I keep hitting the "invalid vscode engine" error - probably because the package has been deprecated in favor of splitting to two packages) I can't add anything to make that happen and to verify that it works correctly.

So I'm getting scope creep.

Let's release a version that doesn't reformat the html.

Then in an upcoming package:

  1. Update vscode to the two new packages officially supported
  2. Consider replacing Pretty with js-beautify
  3. Add pre/code tags to the sample homepage in the tests, and see how that is formatted.
no-stack-dub-sack commented 2 years ago

Got it... Can you come up with a quick example class with comments with examples that break in the current apexdoc setup? I'd like to run ApexDox on it against my local build both with and without Pretty, then I can share the two output HTML files here. I've done a test without Pretty locally and it def changes the output HTML quite a bit, but its not absolutely terrible either. In any case, maybe it will be worth getting that example in there. It will both help to prove that pretty is the culprit and get a good test case in there to make sure nothing breaks it in the future.

Once we're satisfied that Pretty is the culprit, we can add in this class (we don't necessarily need to create tests for it, just adding it may be ok so we can get a visual inspection of the HTML both as source and in the browser) and merge in this PR.

Though if you're unable to run the tests, I'll have to branch off of your branch, run the tests on the new branch and commit the new snapshots. That will be the branch we end up merging in. Your commits will still go in as authored by you.

Also, no need to add the example file to source code yet, just sharing here in a comment will be fine.

dschach commented 2 years ago

Thanks. That sounds great. Here's a specific html page that is causing problems. triggerhandlerhome. As you can see in the project, I have had to put in br tags, and then I run a script after creating the docs to clean them up, as the ApexDox extension doesn't respect line breaks in my pre/code blocks. In short, the ApexDox extension doesn't respect pre/code blocks. Maybe going straight to js-beautify would work better? Or I'd say leave it alone and let Prettier, which many projects use anyway, handle it.

In a convoluted process, I use husky to run the pre-commit file, which

My advice would be to fork my trigger handler repo https://github.com/dschach/salesforce-trigger-framework and then to run ApexDox and see what happens to the files.

Please ignore that I overwrote some of your files - it's also a test-ground for me looking into writing an Apex language file for highlight.js, but that can be updated separately. I'm still unsure why tests don't always pass, so maybe we can do a live coding session to look into it together. Maybe it's my extensions or settings?

no-stack-dub-sack commented 2 years ago

I will look into it, though I think its strange, as you can see from this sample here (with no post-processing on the HTML), the sample code appears to be formatted fine: https://apexdox-sample-docs.surge.sh/BotField.html

I will look into it and see what I can figure out

no-stack-dub-sack commented 2 years ago

Also, is triggerhandlerhome.html is the docs home page, right? In other words, this file is hand-written, and not actually generated by ApexDox. Is this correct?

dschach commented 2 years ago

homePage.html is the docs home page, and the other one is group-content. And yes, both pages are hand-written and meant for display as the home page and when looking at the docs. https://dschach.github.io/salesforce-trigger-framework/index.html (Ignore the code coloring - that's me doing some side work on highlight.js)

dschach commented 2 years ago

Sample code in @example tags is fine. It's the code in the homepage and other imported html page that is odd.

no-stack-dub-sack commented 2 years ago

Ok, this info was really helpful. I thought it was code from @example blocks that was not being formatted correctly. I can now say with 100% certainty the the Pretty plugin/formatter is not the issue. Both with and without pretty, when any supplementary HTML files are processed, the whitespace within any <pre><code> blocks is not respected. Not exactly sure what the problem is yet, but I can tell you its not pretty.

With that said, I see no reason to remove it. But I do plan on getting to the heart of the problem so the issue will be fixed in any case.

Don't make any changes to your PR just yet. Let me first investigate the issue and then suggest a path forward, as I've also merged in all the dependabot changes to the stage branch which may have some conflicts with yarn.lock in this branch.

Let me figure out the issue, then we'll figure out exactly what's important and what's not for the next release.

no-stack-dub-sack commented 2 years ago

Ok, now that I fully understand the problem, I'm pretty sure I've been able to identify the root cause. This line right here: https://github.com/no-stack-dub-sack/apexdox-vs-code/blob/6a125f1780c6f41dba76da14b02d1b5e521c42af/src/common/LineReader.ts#L77

Once I understood what was going on, I had a hunch that the problem was with the LineReader class which is responsible for parsing any supplementary content. In FileManager.ts, we construct a line reader here and call toString() on it: https://github.com/no-stack-dub-sack/apexdox-vs-code/blob/6a125f1780c6f41dba76da14b02d1b5e521c42af/src/engine/FileManager.ts#L150 Basically, when we construct line reader, it just splits a file up line by line. And as you can see from the previous link, the toString() method just puts all the split up lines together in a single string / line (it optionally will trim each line, but we're not doing that here). See where I'm going with this, though? By parsing the supplementary HTML files this way we're completely doing away with any user-defined new lines, regardless of what tag its in....

So... in order to fix this issue, we need a different way to handle user-defined HTML content or this problem will remain, regardless of Pretty.

no-stack-dub-sack commented 2 years ago

I believe this change fixes the issue:

image

This ensures the split up file is re-joined with newline chars, and does not trim each line, so user-defined new lines and white space will always be respected. I've tested this with code samples in the homepage file with pretty still enabled and it works fine now. Let me take another look at this PR, and then think about what makes sense to include now that we know the issue and the fix.

dschach commented 2 years ago

WONDERFUL! That is great. I think we've solved a big part of the problem. I'd recommend updating the h3/h2 situation now, and once you've got that working, I will submit a small separate PR to update Highlight.js to the latest version. After that, I may suggest the ability to include custom html (yes, a security hole) on every page head section, to be included without any changes/formatting, so I can, for example, add additional JS or CSS. But let's cross those bridges later. I'm really happy that you found the solution!

dschach commented 2 years ago

The H3/H2 mismatch is only for html with class subsection-title.

dschach commented 2 years ago

@no-stack-dub-sack I think that's it - I've restored the proper files, and even though yarn.lock may be off, it should be easy enough to fix that. I put in your linereader change and restored Pretty.

no-stack-dub-sack commented 2 years ago

Remind me again what the motivation is for updating highlight.js?

dschach commented 2 years ago

Good question. Latest version allows us to use an Apex-specific highlighter that I'm working on, and I think it will make all the places we show code look properly colored. Technically not essential, but I think it is good for something like this to be updated, especially with so many changes since version 9.

Or we can leave it as is. I'd still recommend putting a place where people can insert custom HTML in the head element.

I'm not always a "latest and greatest" person, but this looks to be a simple upgrade that I've been using with great success by overriding the default library, so I can confirm that it doesn't break anything (as long as we update index.js as well).

Sent via mobile. Please excuse brevity.

On Fri, Apr 1, 2022, 11:08 Peter Weinberg @.***> wrote:

Remind me again what the motivation is for updating highlight.js?

— Reply to this email directly, view it on GitHub https://github.com/no-stack-dub-sack/apexdox-vs-code/pull/44#issuecomment-1086193771, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAE3QMM6ZCGIIQYN5AWB5U3VC43R7ANCNFSM5RMLWZFQ . You are receiving this because you authored the thread.Message ID: @.***>

no-stack-dub-sack commented 2 years ago

To make this easier, now that dependabot changes have been merged into stage, this is what I'd like to happen:

The CodeQL action can go in a different PR.

Sorry for having you jump through all these hoops, but it will be easier in the end. If you don't want to spend the extra time, I'd understand (since you've already spent a lot already), and I'd be happy to make these changes on my own.

dschach commented 2 years ago

I like that plan.

Sent via mobile. Please excuse brevity.

On Fri, Apr 1, 2022, 11:15 Peter Weinberg @.***> wrote:

To make this easier, now that dependabot changes have been merged into stage, this is what I'd like to happen:

  • Please close this PR
  • Create a branch off of my stage branch
  • Make changes to fix H2/H3 issue, but do not update any of the files in src/test/snapshots. These files will be updated programmatically after the tests are successfully run with the update-snapshots command
  • Make line reader changes
  • Open a new PR with these changes only (there should be no changes to yarn.lock - since you're not running the tests locally, once you branch off of stage, no need to even run yarn install)
  • Once the PR is open, I'll add an additional commit on top of yours to update the snapshots (since I'm the only one able to run the tests at the moment)

The CodeQL action can go in a different PR.

Sorry for having you jump through all these hoops, but it will be easier in the end. If you don't want to spend the extra time, I'd understand (since you've already spent a lot already), and I'd be happy to make these changes on my own.

— Reply to this email directly, view it on GitHub https://github.com/no-stack-dub-sack/apexdox-vs-code/pull/44#issuecomment-1086199134, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAE3QMKSFMTM6RONIC4Z27DVC44MDANCNFSM5RMLWZFQ . You are receiving this because you authored the thread.Message ID: @.***>