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

Parse Ruby comments as CommonMark #2562

Closed
wants to merge 5 commits into from
Closed

Parse Ruby comments as CommonMark #2562

wants to merge 5 commits into from

Conversation

evaogbe
Copy link
Contributor

@evaogbe evaogbe commented Feb 12, 2019

Updates the formatting of Ruby comments to expect comments in the format
of CommonMark. Proto comments generally
follow the CommonMark spec.

Replaces ad hoc comment transformations with a visitor for the Markdown
elements. Adds tests for the RubyCommentReformatter.

Cleans up comment regex patterns.

Updates #722

@evaogbe
Parse Ruby comments as CommonMark
3981768 
Updates the formatting of Ruby comments to expect comments in the format 
of [CommonMark](https://commonmark.org/). Proto comments generally 
follow the CommonMark spec.

Replaces ad hoc comment transformations with a visitor for the Markdown 
elements. Adds tests for the RubyCommentReformatter.

Cleans up comment regex patterns.

Updates googleapis#722
@evaogbe evaogbe requested a review from andreamlin February 12, 2019 20:59
@andreamlin andreamlin requested a review from landrito February 12, 2019 21:04
@andreamlin
Copy link
Contributor

andreamlin commented Feb 12, 2019 •
edited
Loading

seems good to me, but I completely lack contextual knowledge about commonmark

+ @landrito

edit: want to look into the CI failures?

@andreamlin
Copy link
Contributor

maybe the failures are DIRT related?

@codecov
Copy link

codecov bot commented Feb 13, 2019

Codecov Report

Merging #2562 into master will increase coverage by 0.54%.
The diff coverage is 90.32%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2562      +/-   ##
============================================
+ Coverage     86.54%   87.08%   +0.54%     
- Complexity     5464     5508      +44     
============================================
  Files           465      465              
  Lines         21768    21847      +79     
  Branches       2391     2393       +2     
============================================
+ Hits          18839    19026     +187     
+ Misses         2086     1950     -136     
- Partials        843      871      +28
Impacted Files Coverage Δ Complexity Δ
.../java/com/google/api/codegen/util/LinkPattern.java 100% <100%> (ø) 4 <1> (ø) ⬇️
.../api/codegen/util/ruby/RubyCommentReformatter.java 89.91% <89.56%> (-8.5%) 3 <2> (-16)
...a/com/google/api/codegen/util/MultiYamlReader.java 0% <0%> (-58.83%) 0% <0%> (-4%)
...om/google/api/codegen/util/CommentTransformer.java 96.29% <0%> (-3.71%) 5% <0%> (ø)
...api/codegen/config/ResourceNameMessageConfigs.java 91.3% <0%> (-1.45%) 24% <0%> (-1%)
...google/api/codegen/configgen/ConfigYamlReader.java 36.66% <0%> (ø) 2% <0%> (?)
.../google/api/codegen/config/GapicProductConfig.java 77.03% <0%> (+0.47%) 69% <0%> (ø) ⬇️
...api/codegen/discogapic/DiscoGapicGeneratorApp.java 56.17% <0%> (+2.33%) 6% <0%> (ø) ⬇️
...com/google/api/codegen/configgen/ConfigHelper.java 36.84% <0%> (+15.78%) 4% <0%> (+3%) ⬆️
...google/api/codegen/configgen/MessageGenerator.java 58.03% <0%> (+58.03%) 30% <0%> (+30%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb2c4a9...bfaa0f2. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 13, 2019 •
edited
Loading

Codecov Report

Merging #2562 into master will increase coverage by 0.54%.
The diff coverage is 90.32%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2562      +/-   ##
============================================
+ Coverage     86.54%   87.08%   +0.54%     
- Complexity     5465     5508      +43     
============================================
  Files           465      465              
  Lines         21767    21847      +80     
  Branches       2391     2393       +2     
============================================
+ Hits          18838    19026     +188     
+ Misses         2087     1950     -137     
- Partials        842      871      +29
Impacted Files Coverage Δ Complexity Δ
.../java/com/google/api/codegen/util/LinkPattern.java 100% <100%> (ø) 4 <1> (ø) ⬇️
.../api/codegen/util/ruby/RubyCommentReformatter.java 89.91% <89.56%> (-8.5%) 3 <2> (-16)
...a/com/google/api/codegen/util/MultiYamlReader.java 0% <0%> (-58.83%) 0% <0%> (-4%)
...oogle/api/codegen/util/java/JavaNameFormatter.java 72% <0%> (-4%) 14% <0%> (-1%)
...om/google/api/codegen/util/CommentTransformer.java 96.29% <0%> (-3.71%) 5% <0%> (ø)
.../transformer/java/JavaSampleImportTransformer.java 89.88% <0%> (-2.25%) 25% <0%> (-1%)
...api/codegen/config/ResourceNameMessageConfigs.java 91.3% <0%> (-1.45%) 24% <0%> (-1%)
.../google/api/codegen/config/GapicProductConfig.java 77.03% <0%> (-0.24%) 69% <0%> (-1%)
...m/google/api/codegen/transformer/SurfaceNamer.java 71.53% <0%> (-0.24%) 203% <0%> (-1%)
...google/api/codegen/configgen/ConfigYamlReader.java 36.66% <0%> (ø) 2% <0%> (?)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 761abeb...c350ef4. Read the comment docs.

@evaogbe
Copy link
Contributor Author

evaogbe commented Feb 25, 2019

@landrito PTAL

@evaogbe evaogbe requested review from jbolinger and removed request for landrito March 28, 2019 21:22
@jbolinger
Copy link
Contributor

@eoogbe what's the reason for this change? The baseline changes don't look too significant but I'm worried that this could break existing synth scripts for the Ruby libraries.

@evaogbe
Copy link
Contributor Author

evaogbe commented Mar 28, 2019

It ensures the Ruby comments consistently follow CommonMark, which the form of markdown proto comments use. The main reason for the change is to make the CommentReformatter more testable. See #722

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 28, 2019
jbolinger
jbolinger suggested changes Mar 29, 2019
View reviewed changes
Copy link
Contributor

@jbolinger jbolinger left a comment

Choose a reason for hiding this comment

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

It looks like this changes the output, particularly with how whitespace is handled. I'm concerned that this is going to create churn in our existing libraries. The extra tests are nice but I'm not sure it's worth it given that we'll be moving to micro-generators soon, unless we don't need to change the existing behavior.

src/main/java/com/google/api/codegen/util/ruby/RubyCommentReformatter.java
// Might as well create only one. Parser is thread-safe.
private static final Parser PARSER = Parser.builder().build();

private static String CLOUD_URL_PREFIX = "https://cloud.google.com";
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this duplicated now?

src/test/java/com/google/api/codegen/gapic/testdata/ruby/ruby_library.baseline
# can use the Joda Time's [`ISODateTimeFormat.dateTime()`](
# http://www.joda.org/joda-time/apidocs/org/joda/time/format/ISODateTimeFormat.html#dateTime--
# ) to obtain a formatter capable of generating timestamps in this format.
# can use the Joda Time's [`ISODateTimeFormat.dateTime()`](http://www.joda.org/joda-time/apidocs/org/joda/time/format/ISODateTimeFormat.html#dateTime--) to obtain a formatter capable of generating timestamps in this format.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this changing?

src/test/java/com/google/api/codegen/gapic/testdata/ruby/ruby_library.baseline
@@ -1991,7 +1984,9 @@ module Google
#
# * Here is a sentence about the first element of the list that continues
# into a second line.
#
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these newlines being added?

@evaogbe
Copy link
Contributor Author

evaogbe commented Mar 29, 2019

@jbolinger If it's the case that we don't want to fix the comment formatting, then we should probably close the issue.

@evaogbe evaogbe closed this Mar 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jbolinger jbolinger jbolinger requested changes

@andreamlin andreamlin Awaiting requested review from andreamlin

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@evaogbe @andreamlin @jbolinger @googlebot