ctubbsii commented on code in PR #14:
URL: https://github.com/apache/thrift-website/pull/14#discussion_r3018551042
##########
.github/workflows/jekyll.yaml:
##########
@@ -21,29 +21,28 @@ name: CI
on:
push:
- branches: [ 'main' ]
+ branches: ["main"]
pull_request:
- branches: [ 'main' ]
+ branches: ["main"]
jobs:
jekyll:
timeout-minutes: 15
runs-on: ubuntu-latest
steps:
- - uses: actions/checkout@v4
- - name: Set up Ruby
- uses: ruby/setup-ruby@v1
- with:
- ruby-version: 3.2.2
- bundler-cache: true
- - name: Test site build
- run: |
- ruby --version
- bundle exec jekyll build
- - name: Upload site
- uses: actions/upload-artifact@v4
- with:
- name: site
- path: ./_site/
- if-no-files-found: ignore
-
+ - uses: actions/checkout@v4
+ - name: Set up Ruby
+ uses: ruby/setup-ruby@v1
+ with:
+ ruby-version: 4.0.2
Review Comment:
This only updates the ruby version for the test environment. This is not the
version that gets used during the site build. The version that gets used for
that is whatever is installed on the ASF buildbot nodes that build jekyll
sites. The minimum version should be updated in the Gemfile to whatever is
needed for these specific gems. I think the version that's there now is too old.
##########
README.md:
##########
@@ -131,7 +135,6 @@ pushes.
The final site can be viewed [here][production].
-
Review Comment:
The extra blank line is to visually separate the footer links from the
actual markdown content. It should not be removed.
##########
Gemfile:
##########
@@ -5,3 +5,8 @@ gem 'jekyll-redirect-from', '>= 0.16.0'
gem "webrick", "~> 1.8"
gem "google-protobuf", "3.25.5"
+
+# BigDecimal is required for liquid, but it does not declare a dependency on it
+gem "bigdecimal"
+# logger is required for jekyll, but it does not declare a dependency on it
+gem "logger"
Review Comment:
Are you sure about these? I modeled this build after Apache Accumulo's
website build, and it brings in these dependencies just fine without an
explicit declaration.
Also, it is no longer necessary to include the webrick and google-protobuf.
Those were locked due to the ASF buildbot servers being stuck on an older
version of ruby. But now, you can use `ruby '>=3.2'` and get the newer versions
of webrick and google-protobuf that work just fine.
##########
_plugins/remote_snippets.rb:
##########
@@ -21,44 +22,84 @@ def initialize(tag_name, text, tokens)
def render(context)
title = context.registers[:site].config['title']
- prefix = context.registers[:site].config['gitbox_url']
- url = "#{prefix};a=blob_plain;hb=HEAD;f=#{@path}"
- pretty_url = "#{prefix};a=blob;hb=HEAD;f=#{@path}"
- content = ""
- if @range == "all"
- URI.open(url) {|f| content = f.read }
- else
- rangenums = @range.split(",", 2)
- first = 0
- second = 0
- if rangenums.length() == 2
- first = Integer(rangenums[0])
- second = Integer(rangenums[1]) - first
- URI.open(url) {|f| content =
f.each_line.drop(first).take(second).join() }
- else
- first = Integer(rangenums[0])
- URI.open(url) {|f| content = f.each_line.drop(first).join() }
- end
- end
+ source = snippet_source(context)
+ content = read_content(source[:read_path], source[:local])
snippet_type = "snippet"
if @type == "direct"
- content = Kramdown::Document.new(content).to_html
+ content = render_markdown(content, context)
snippet_type = "page"
else
content = content.force_encoding("utf-8")
formatter = Rouge::Formatters::HTMLLegacy.new
lexer = Rouge::Lexer.find_fancy(@type, content)
content = formatter.format(lexer.lex(content))
end
+ source_link = "<a
href=\"#{source[:pretty_url]}\">#{CGI.escapeHTML(@path)}</a>"
Review Comment:
Is it necessary to create a separate variable for this one-time use?
##########
Gemfile.lock:
##########
@@ -145,13 +148,82 @@ PLATFORMS
x86_64-linux-musl
DEPENDENCIES
+ bigdecimal
google-protobuf (= 3.25.5)
jekyll (>= 4.2.0)
jekyll-redirect-from (>= 0.16.0)
+ logger
webrick (~> 1.8)
+CHECKSUMS
+ addressable (2.8.8)
sha256=7c13b8f9536cf6364c03b9d417c19986019e28f7c00ac8132da4eb0fe393b057
+ base64 (0.3.0)
sha256=27337aeabad6ffae05c265c450490628ef3ebd4b67be58257393227588f5a97b
+ bigdecimal (4.0.0)
sha256=ec797dffef5566eeccf9f1a39df8fc78ef9c39158dc85b2036d5f11c7e0d3b86
+ colorator (1.1.0)
sha256=e2f85daf57af47d740db2a32191d1bdfb0f6503a0dfbc8327d0c9154d5ddfc38
+ concurrent-ruby (1.3.6)
sha256=6b56837e1e7e5292f9864f34b69c5a2cbc75c0cf5338f1ce9903d10fa762d5ab
+ csv (3.3.5)
sha256=6e5134ac3383ef728b7f02725d9872934f523cb40b961479f69cf3afa6c8e73f
+ em-websocket (0.5.3)
sha256=f56a92bde4e6cb879256d58ee31f124181f68f8887bd14d53d5d9a292758c6a8
+ eventmachine (1.2.7)
sha256=994016e42aa041477ba9cff45cbe50de2047f25dd418eba003e84f0d16560972
+ ffi (1.17.2)
sha256=297235842e5947cc3036ebe64077584bff583cd7a4e94e9a02fdec399ef46da6
+ ffi (1.17.2-aarch64-linux-gnu)
sha256=c910bd3cae70b76690418cce4572b7f6c208d271f323d692a067d59116211a1a
+ ffi (1.17.2-aarch64-linux-musl)
sha256=69e6556b091d45df83e6c3b19d3c54177c206910965155a6ec98de5e893c7b7c
+ ffi (1.17.2-arm-linux-gnu)
sha256=d4a438f2b40224ae42ec72f293b3ebe0ba2159f7d1bd47f8417e6af2f68dbaa5
+ ffi (1.17.2-arm-linux-musl)
sha256=977dfb7f3a6381206dbda9bc441d9e1f9366bf189a634559c3b7c182c497aaa3
+ ffi (1.17.2-arm64-darwin)
sha256=54dd9789be1d30157782b8de42d8f887a3c3c345293b57ffb6b45b4d1165f813
+ ffi (1.17.2-x86-linux-gnu)
sha256=95d8f9ebea23c39888e2ab85a02c98f54acb2f4e79b829250d7267ce741dc7b0
+ ffi (1.17.2-x86-linux-musl)
sha256=41741449bab2b9530f42a47baa5c26263925306fad0ac2d60887f51af2e3b24c
+ ffi (1.17.2-x86_64-darwin)
sha256=981f2d4e32ea03712beb26e55e972797c2c5a7b0257955d8667ba58f2da6440e
+ ffi (1.17.2-x86_64-linux-gnu)
sha256=05d2026fc9dbb7cfd21a5934559f16293815b7ce0314846fee2ac8efbdb823ea
+ ffi (1.17.2-x86_64-linux-musl)
sha256=97c0eb3981414309285a64dc4d466bd149e981c279a56371ef811395d68cb95c
+ forwardable-extended (2.6.0)
sha256=1bec948c469bbddfadeb3bd90eb8c85f6e627a412a3e852acfd7eaedbac3ec97
+ google-protobuf (3.25.5)
sha256=4333fe2e9009131d8bec9b4ffcfae7b5a0a2d1bd18061e8aaaf0fd3d5c835639
+ http_parser.rb (0.8.0)
sha256=5a0932f1fa82ce08a8516a2685d5a86031c000560f89946913c555a0697544be
+ i18n (1.14.7)
sha256=ceba573f8138ff2c0915427f1fc5bdf4aa3ab8ae88c8ce255eb3ecf0a11a5d0f
+ jekyll (4.4.1)
sha256=4c1144d857a5b2b80d45b8cf5138289579a9f8136aadfa6dd684b31fe2bc18c1
+ jekyll-redirect-from (0.16.0)
sha256=6635cae569ef9b0f90ffb71ec014ba977177fafb44d32a2b0526288d4d9be6db
+ jekyll-sass-converter (3.1.0)
sha256=83925d84f1d134410c11d0c6643b0093e82e3a3cf127e90757a85294a3862443
+ jekyll-watch (2.2.1)
sha256=bc44ed43f5e0a552836245a54dbff3ea7421ecc2856707e8a1ee203a8387a7e1
+ json (2.18.0)
sha256=b10506aee4183f5cf49e0efc48073d7b75843ce3782c68dbeb763351c08fd505
+ kramdown (2.5.1)
sha256=87bbb6abd9d3cebe4fc1f33e367c392b4500e6f8fa19dd61c0972cf4afe7368c
+ kramdown-parser-gfm (1.1.0)
sha256=fb39745516427d2988543bf01fc4cf0ab1149476382393e0e9c48592f6581729
+ liquid (4.0.4)
sha256=4fcfebb1a045e47918388dbb7a0925e7c3893e58d2bd6c3b3c73ec17a2d8fdb3
+ listen (3.9.0)
sha256=db9e4424e0e5834480385197c139cb6b0ae0ef28cc13310cfd1ca78377d59c67
+ logger (1.7.0)
+ mercenary (0.4.0)
sha256=b25a1e4a59adca88665e08e24acf0af30da5b5d859f7d8f38fba52c28f405138
+ pathutil (0.16.2)
sha256=e43b74365631cab4f6d5e4228f812927efc9cb2c71e62976edcb252ee948d589
+ public_suffix (7.0.0)
sha256=f7090b5beb0e56f9f10d79eed4d5fbe551b3b425da65877e075dad47a6a1b095
+ rb-fsevent (0.11.2)
sha256=43900b972e7301d6570f64b850a5aa67833ee7d87b458ee92805d56b7318aefe
+ rb-inotify (0.11.1)
sha256=a0a700441239b0ff18eb65e3866236cd78613d6b9f78fea1f9ac47a85e47be6e
+ rexml (3.4.4)
sha256=19e0a2c3425dfbf2d4fc1189747bdb2f849b6c5e74180401b15734bc97b5d142
+ rouge (4.6.1)
sha256=5075346d5797d6864be93f7adc75a16047a7dbfa572c63c502419ffa582c77de
+ safe_yaml (1.0.5)
sha256=a6ac2d64b7eb027bdeeca1851fe7e7af0d668e133e8a88066a0c6f7087d9f848
+ sass-embedded (1.77.5-aarch64-linux-android)
sha256=ba9c75b59b34e12679c55a6a42da5b8b90311bd4e41477ff7d34a3738d2bf2e0
+ sass-embedded (1.77.5-aarch64-linux-gnu)
sha256=c84eda6b86669a15695d9a7ddbc7a6e3cb706735d17fe37c5c4e1c584d467534
+ sass-embedded (1.77.5-aarch64-linux-musl)
sha256=6f6224df2d4a22ba686a636ea8a4a7f146208d9e3dbbe35041a7daed63528c40
+ sass-embedded (1.77.5-aarch64-mingw-ucrt)
sha256=66abfce03d940ca5c231c5eaf3df658508761dac09f7474101db9ee549561080
+ sass-embedded (1.77.5-arm-linux-androideabi)
sha256=f5c55676da02a33d664d3ebef043e6b3813d64fd4fc1016420dd8ed4c5b252bf
+ sass-embedded (1.77.5-arm-linux-gnueabihf)
sha256=e0e067f06ea4bb001c7c2099d6f0980d1fad918b3ea182857563f6488f07ff65
+ sass-embedded (1.77.5-arm-linux-musleabihf)
sha256=ad2b7cf152d5a7195d57508d466ebce56cdcac5d3ef95f05fa62664eeb9a5eda
+ sass-embedded (1.77.5-arm64-darwin)
sha256=a6a524aefe8b181c55d5f11c26b2329a06e20de32180f18904fc8488b89bdb36
+ sass-embedded (1.77.5-riscv64-linux-android)
sha256=04cd2e96f12e1fc84010a01d213dd4496afbd7c92ed3f72b904a5cf55b8048c7
+ sass-embedded (1.77.5-riscv64-linux-gnu)
sha256=7527f7eb49dc788892a2db4e22d23d06bbce05ddf6663950069cd8f933005484
+ sass-embedded (1.77.5-riscv64-linux-musl)
sha256=b658e258db2eaa183143a12e95fa229b97f39fb74fa0d4463899bfbb05dba375
+ sass-embedded (1.77.5-x86-cygwin)
sha256=834ee1a4cb4be1d354f12422550ada9db5966a836c6af05c2a2e7c3aa3790117
+ sass-embedded (1.77.5-x86-linux-android)
sha256=4b0334b50a5475bcb621257abc4b32ca802205dbb23608a78c60b1ce1745d4b3
+ sass-embedded (1.77.5-x86-linux-gnu)
sha256=de9255b988ce9866a4734acfcb44f55e73055607e11a1cfa4a0105e5fd6fd928
+ sass-embedded (1.77.5-x86-linux-musl)
sha256=8fc2a3dda88a903550f422c99a0bab9385af493d731f2256cd580fe4a4e92423
+ sass-embedded (1.77.5-x86-mingw-ucrt)
sha256=ea7544b5b30d64a4c9547d576eb2e76f66de8d0d773cfe205357cf564cfae773
+ sass-embedded (1.77.5-x86_64-cygwin)
sha256=11eeea03f15ae39b03a84bebf8d80f8824941d2cfde57b1c21dbb31d65de0dc3
+ sass-embedded (1.77.5-x86_64-darwin)
sha256=228ee25c012e50bef83e433415b20dddaaaa9c01c8c40c4f99669d919f26bfc3
+ sass-embedded (1.77.5-x86_64-linux-android)
sha256=e183bf5a0b3916eb3f5b3c7596ae44fd72edb0fe233dd00df48131271ed752ea
+ sass-embedded (1.77.5-x86_64-linux-gnu)
sha256=3000a6b984ea746eac1f0c9a17400134fe76c3d41b33436262ac82b5d153d7b4
+ sass-embedded (1.77.5-x86_64-linux-musl)
sha256=c5dd43155112f7b4eb9e3cafb8ceaa54a157977a2033b1022875bac71a4a4d56
+ terminal-table (3.0.2)
sha256=f951b6af5f3e00203fb290a669e0a85c5dd5b051b3b023392ccfd67ba5abae91
+ unicode-display_width (2.6.0)
sha256=12279874bba6d5e4d2728cef814b19197dbb10d7a7837a869bab65da943b7f5a
+ webrick (1.9.2)
sha256=beb4a15fc474defed24a3bda4ffd88a490d517c9e4e6118c3edce59e45864131
+
RUBY VERSION
- ruby 3.3.4p94
+ ruby 3.3.4p94
BUNDLED WITH
- 2.5.22
+ 4.0.9
Review Comment:
This version of bundler is much much newer, and I don't think it's the
version that's on the ASF buildbot servers. I'd be cautious about using that
newer bundler version, and instead just use `bundle update` using the current
version.
##########
_plugins/remote_snippets.rb:
##########
@@ -21,44 +22,84 @@ def initialize(tag_name, text, tokens)
def render(context)
title = context.registers[:site].config['title']
- prefix = context.registers[:site].config['gitbox_url']
- url = "#{prefix};a=blob_plain;hb=HEAD;f=#{@path}"
- pretty_url = "#{prefix};a=blob;hb=HEAD;f=#{@path}"
- content = ""
- if @range == "all"
- URI.open(url) {|f| content = f.read }
- else
- rangenums = @range.split(",", 2)
- first = 0
- second = 0
- if rangenums.length() == 2
- first = Integer(rangenums[0])
- second = Integer(rangenums[1]) - first
- URI.open(url) {|f| content =
f.each_line.drop(first).take(second).join() }
- else
- first = Integer(rangenums[0])
- URI.open(url) {|f| content = f.each_line.drop(first).join() }
- end
- end
+ source = snippet_source(context)
+ content = read_content(source[:read_path], source[:local])
snippet_type = "snippet"
if @type == "direct"
- content = Kramdown::Document.new(content).to_html
+ content = render_markdown(content, context)
snippet_type = "page"
else
content = content.force_encoding("utf-8")
formatter = Rouge::Formatters::HTMLLegacy.new
lexer = Rouge::Lexer.find_fancy(@type, content)
content = formatter.format(lexer.lex(content))
end
+ source_link = "<a
href=\"#{source[:pretty_url]}\">#{CGI.escapeHTML(@path)}</a>"
#content = CGI::escapeHTML(content)
return """
#{content}
<p class=\"snippet_footer\">This #{snippet_type} was generated by #{title}'s
<strong>source tree docs</strong>:
-<a href=\"#{pretty_url}\">#{@path}</a>
+#{source_link}
</p>
"""
end
+
+ private
+
+ def snippet_source(context)
Review Comment:
It may be useful to break out the code into separate functions that do fewer
things, but it makes reviewing this PR substantially more difficult. It also
increases the code complexity by changing 22 lines of easy-to-read code into 5
separate functions that are only each used once, spanning 56 lines of code that
are a bit more complicated to follow.
I strongly suggest doing the code reorganization in a separate PR from the
narrow bug fix you're trying to accomplish here.
##########
_plugins/remote_snippets.rb:
##########
@@ -21,44 +22,84 @@ def initialize(tag_name, text, tokens)
def render(context)
title = context.registers[:site].config['title']
- prefix = context.registers[:site].config['gitbox_url']
- url = "#{prefix};a=blob_plain;hb=HEAD;f=#{@path}"
- pretty_url = "#{prefix};a=blob;hb=HEAD;f=#{@path}"
- content = ""
- if @range == "all"
- URI.open(url) {|f| content = f.read }
- else
- rangenums = @range.split(",", 2)
- first = 0
- second = 0
- if rangenums.length() == 2
- first = Integer(rangenums[0])
- second = Integer(rangenums[1]) - first
- URI.open(url) {|f| content =
f.each_line.drop(first).take(second).join() }
- else
- first = Integer(rangenums[0])
- URI.open(url) {|f| content = f.each_line.drop(first).join() }
- end
- end
+ source = snippet_source(context)
+ content = read_content(source[:read_path], source[:local])
snippet_type = "snippet"
if @type == "direct"
- content = Kramdown::Document.new(content).to_html
+ content = render_markdown(content, context)
snippet_type = "page"
else
content = content.force_encoding("utf-8")
formatter = Rouge::Formatters::HTMLLegacy.new
lexer = Rouge::Lexer.find_fancy(@type, content)
content = formatter.format(lexer.lex(content))
end
+ source_link = "<a
href=\"#{source[:pretty_url]}\">#{CGI.escapeHTML(@path)}</a>"
#content = CGI::escapeHTML(content)
return """
#{content}
<p class=\"snippet_footer\">This #{snippet_type} was generated by #{title}'s
<strong>source tree docs</strong>:
-<a href=\"#{pretty_url}\">#{@path}</a>
+#{source_link}
</p>
"""
end
+
+ private
+
+ def snippet_source(context)
+ prefix = context.registers[:site].config['gitbox_url']
+ checkout = local_checkout_root(context)
+ return remote_source(prefix) unless checkout
+
+ local_path = File.expand_path(@path, checkout)
+ unless local_path.start_with?("#{checkout}/") && File.file?(local_path)
+ raise ArgumentError, "remote_snippet local checkout path is not a file:
#{local_path}"
+ end
+
+ {
+ local: true,
+ read_path: local_path,
+ pretty_url: "#{prefix};a=blob;hb=HEAD;f=#{@path}"
+ }
+ end
+
+ def read_content(path, local)
+ lines = if local
+ File.readlines(path)
+ else
+ URI.open(path) { |file| file.each_line.to_a }
+ end
+ return lines.join() if @range == "all"
+
+ rangenums = @range.split(",", 2)
+ first = Integer(rangenums[0])
+ return lines.drop(first).join() if rangenums.length() == 1
+
+ length = Integer(rangenums[1]) - first
+ lines.drop(first).take(length).join()
+ end
+
+ def local_checkout_root(context)
+ checkout = ENV["THRIFT_CHECKOUT"] ||
context.registers[:site].config["thrift_checkout"]
+ return nil if checkout.nil? || checkout.empty?
+
+ File.expand_path(checkout)
+ end
+
+ def remote_source(prefix)
+ {
+ local: false,
+ read_path: "#{prefix};a=blob_plain;hb=HEAD;f=#{@path}",
+ pretty_url: "#{prefix};a=blob;hb=HEAD;f=#{@path}"
+ }
+ end
+
+ def render_markdown(content, context)
+ context.registers[:site]
+ .find_converter_instance(Jekyll::Converters::Markdown)
+ .convert(content.to_s)
+ .gsub(/(<(?:div|pre)\s+)class="highlight"/, '\1class="codehilite"')
Review Comment:
Why alter the default syntax highlighting class? Why would the Markdown
converter loaded here use the wrong highlight class? Is this because the
GitHub-flavored Markdown renders the syntax highlighting differently than
Jekyll's Markdown converter?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
