Skip to content
This repository has been archived by the owner on Aug 9, 2022. It is now read-only.

Fix encoding error #63

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Fix encoding error #63

wants to merge 5 commits into from

Conversation

cary-hu
Copy link

@cary-hu cary-hu commented Apr 2, 2021

We are using docfx-doclet for generate java doc to yaml file.

Everything works fine, but when there are some none-ascii character in doc, the yaml file will show the unicode character.
like below:
image

This commit fixes the bug about encoding/decoding issue.

We have already packaged a new doclet jar, and worked with none-ascii character fine,
but we don't know why I can't run the unittest, there are build result(even using the original code):

Results :

Failed tests:   testFilesGeneration(com.microsoft.doclet.DocletRunnerTest): Wrong file content for file target\test-out\com.microsoft.samples.agreements.AgreementDetailsCollectionOperations.AgreementDetailsCollectionOperations.yml(.
.)
  convertHtmlToMarkdown(com.microsoft.util.YamlUtilTest): Wrong result(..)

Tests in error:
  addConstructorsInfoWhenOnlyDefaultConstructor(com.microsoft.build.YmlFilesBuilderTest): (..)
  expandComplexGenericsInReferences(com.microsoft.build.YmlFilesBuilderTest): (..)
  determineUidByLinkContent(com.microsoft.build.YmlFilesBuilderTest): (..)
  buildRefItem(com.microsoft.build.YmlFilesBuilderTest): (..)
  addConstructorsInfo(com.microsoft.build.YmlFilesBuilderTest): (..)
  populateUidValues(com.microsoft.build.YmlFilesBuilderTest): (..)
  splitUidWithGenericsIntoClassNames(com.microsoft.build.YmlFilesBuilderTest): (..)
  buildXrefTagWhenLabelPresents(com.microsoft.lookup.BaseLookupTest): (..)
  makeTypeShort(com.microsoft.lookup.BaseLookupTest): (..)
  expandLiteralBody(com.microsoft.lookup.BaseLookupTest): (..)
  buildCodeTag(com.microsoft.lookup.BaseLookupTest): (..)
  testExtractMethods(com.microsoft.lookup.BaseLookupTest): (..)
  replaceLinksAndCodes(com.microsoft.lookup.BaseLookupTest): (..)
  resolve(com.microsoft.lookup.BaseLookupTest): (..)
  buildXrefTag(com.microsoft.lookup.BaseLookupTest): (..)
  determineComment(com.microsoft.lookup.BaseLookupTest): (..)
  determineTypeForEnumConstant(com.microsoft.lookup.ClassItemsLookupTest): (..)
  extractExceptions(com.microsoft.lookup.ClassItemsLookupTest): (..)
  extractReturnForVariableElement(com.microsoft.lookup.ClassItemsLookupTest): (..)
  extractExceptionDescription(com.microsoft.lookup.ClassItemsLookupTest): (..)
  convertFullNameToOverload(com.microsoft.lookup.ClassItemsLookupTest): (..)
  extractParameterDescription(com.microsoft.lookup.ClassItemsLookupTest): (..)
  extractParameters(com.microsoft.lookup.ClassItemsLookupTest): (..)
  extractReturnDescription(com.microsoft.lookup.ClassItemsLookupTest): (..)
  extractReturnForExecutableElement(com.microsoft.lookup.ClassItemsLookupTest): (..)
  determineSuperclass(com.microsoft.lookup.ClassLookupTest): (..)
  determineSuperclassForChildClass(com.microsoft.lookup.ClassLookupTest): (..)
  determineTypeForEnum(com.microsoft.lookup.ClassLookupTest): (..)
  determineClassContentForStaticClass(com.microsoft.lookup.ClassLookupTest): (..)
  determineTypeForInterface(com.microsoft.lookup.ClassLookupTest): (..)
  determineClassContentForEnum(com.microsoft.lookup.ClassLookupTest): (..)
  determineSuperclassForEnum(com.microsoft.lookup.ClassLookupTest): (..)
  determineClassContent(com.microsoft.lookup.ClassLookupTest): (..)
  determineTypeForClass(com.microsoft.lookup.ClassLookupTest): (..)
  determineTypeParameters(com.microsoft.lookup.ClassLookupTest): (..)
  determineClassContentForInterface(com.microsoft.lookup.ClassLookupTest): (..)
  extractPackageContent(com.microsoft.lookup.PackageLookupTest): (..)

Tests run: 79, Failures: 2, Errors: 37, Skipped: 0

[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  14.510 s
[INFO] Finished at: 2021-04-02T11:15:16+08:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:2.12.4:test (default-test) on project docfx-doclet: There are test failures.
[ERROR]
[ERROR] Please refer to C:\Users\admin\Desktop\New folder\docfx-doclet\target\surefire-reports for the individual test results.
[ERROR] -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException

please check

and, anything else problem with merge, please let me know.

@andrei-punko
Copy link
Contributor

andrei-punko commented Apr 3, 2021

You need to cover your change with tests using next approach:

  1. Add some none-ascii character into javadoc comment inside one of classes of package com.microsoft.samples
    (These classes used for generation of yaml files during tests run)
  2. Update expected generated file inside: src/test/resources/expected-generated-files folder
    (These files compared with generated yaml files during tests run)
    (More easy way to do this - is just grab generated files from /target folder and put them inside /expected-generated-files after check are they satisfied your requirements)
  3. Run tests to check are they passed

<dependency>
<groupId>com.overzealous</groupId>
<groupId>com.kotcrab.remark</groupId>
Copy link
Contributor

@andrei-punko andrei-punko Apr 6, 2021

Choose a reason for hiding this comment

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

Yes, seems original artifact was removed from repo, so we should replace it with possible fork.
And it's mandatory change for person who hasn't legacy com.overzealous artifact in local maven repo.

Copy link
Author

Choose a reason for hiding this comment

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

Ok I see

@cary-hu
Copy link
Author

cary-hu commented Apr 6, 2021

We've found that com.microsoft.samples.subpackage.Person class has an error when I run the Tests, error like below:
image

And the # should not be convert to \\#.

So should I continue investigate this test failure or keep the latest result ?

@andrei-punko
Copy link
Contributor

andrei-punko commented Apr 6, 2021

May be reason in another way of escaping # character in new remark library.
If you built DocFx documentation based on generated yamls with this # character and it looks good - seems we could keep it as is and just adjust expectation in yaml.

<artifactId>remark</artifactId>
<version>${remark.version}</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

May be it's reasonable to use existing constant remark.version (with 1.2.0 value) or remove it from properties

@cary-hu
Copy link
Author

cary-hu commented Apr 6, 2021

If you built DocFx documentation based on generated yamls with this # character and it looks good - seems we could keep it as is and just adjust expectation in yaml.

Ok got that

BTW, which version of docfx should I use, I got some error when I try to build doc with yaml #64

@andrei-punko
Copy link
Contributor

Sure, seems we need reply from guys who used doclet on daily basis, @anmeng10101 for example. To determine which docfx version we should use now.
I built docs manually couple year ago, at that moment used "current version"

@anmeng10101
Copy link
Collaborator

Thanks a lot @andrei-punko for the great suggestion. I will add some highlights in README.

@cary-hu I checked with Docfx team, it is the V2 which public for community, unfortunately the latest version of docfx-doclet is for Docfx V3.
I am thinking to meet your requests, we could consider fork the repo and make the Chinese characters handle changes after this commit. Then use the fork repo to generate yaml files for Docfx V2

@cary-hu
Copy link
Author

cary-hu commented Apr 7, 2021

Thanks for your help, Feel free to let me know if I can do anything to accomplish this work.

@andrei-punko
Copy link
Contributor

@anmeng10101 may be you know about some plans to share Docfx V3 to community in future?
And may be it's reasonable to keep separate branch of doclet in repo which will be compatible to Docfx V2 (?)

@andrei-punko
Copy link
Contributor

andrei-punko commented May 7, 2021

According to #56 - seems guys provided working version via dev branch.
So may be reasonable to rework this pull request to merge it into dev branch (?)
@cary-hu could you check your changes applied on dev branch?

@andrei-punko andrei-punko mentioned this pull request Jun 26, 2021
@andrei-punko
Copy link
Contributor

I adapt changes made in this PR for current dev branch in next PR: #67

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants