Fetch docs with git clone instead of GitHub API#4000
Fetch docs with git clone instead of GitHub API#4000Dylan-fa wants to merge 6 commits intoalphagov:mainfrom
Conversation
Docs folder now fetched with git clone and sparse checkout. Tests updated as well to mock local repo docs.
Docs folder now fetched with git clone and sparse checkout. Tests updated as well to mock local repo docs.
|
Hello and welcome! Sorry to have kept you waiting so long. Huge thanks for working on this! 🙇 I (or one of my colleagues) will try to get back to you with a proper review as soon as poss. |
There was a problem hiding this comment.
Thanks so much for this! ❤️
I think the only thing stopping the CI / pre-merge checks from passing is the trivial rubocop error; rubocop -a should sort it out. (At least, the tests passed on my workstation with that change.)
I'll need to find a colleague to give this a second look before merging, but LGTM!
|
Hi @sengi, thanks for the code review, please see my updated change (which should hopefully pass CI now, once the run is approved). |
|
Thanks @Dylan-fa! One of my colleagues will hopefully be able to take a look for you soon so we can get that second approval and then hopefully we'll be good to go 🚀 |
ChrisBAshton
left a comment
ChrisBAshton
left a comment
There was a problem hiding this comment.
Thanks very much for raising this, @Dylan-fa ⭐
It's looking good (with some real improvements to our tests!). I've just got some minor comments to address below. I'll then give it a quick manual test before approval 👍
PS: if possible, please squash all your commits together; there's not much value in keeping 'fixup' and merge commits.
| # Lorem ipsum | ||
|
|
||
| ## tl;dr | ||
|
|
||
| Lorem ipsum dolor sit amet, consectetur adipiscing elit. Nam efficitur lacus in | ||
| neque consequat porta, [Suspendisse iaculis](#suspendisse-iaculis). Sed et | ||
| [inline link](./inline-link.md), imperdiet eros ut, [aliased link]. | ||
| [Absolute link](https://nam.com/eget/dui/absolute-link.md) | ||
|
|
||
| [localhost](localhost:999) | ||
|
|
||
| [Relative docs link with period](./docs/prefixed.md) | ||
|
|
||
| [Relative docs link without period](docs/no-prefix.md) | ||
|
|
||
| [Link relative to root](/public/json_examples/requests/foo.json) | ||
|
|
||
| [Subfolder](docs/some-subfolder/foo.md) | ||
|
|
||
| Visit “http://localhost:3108” | ||
|
|
||
| ## Suspendisse iaculis | ||
|
|
||
|  | ||
|
|
||
| [aliased link]: ./lib/aliased_link.rb | ||
|
|
||
| ## Other headings to test | ||
|
|
||
| ### data.gov.uk | ||
|
|
||
| ### Patterns & Style Guides |
There was a problem hiding this comment.
The contents of this file aren't under test, as far as I can see. I think we can just empty the file (or replace the contents with just a string like "placeholder text"). This will make it clearer to devs that the two spec fixture files don't need to be kept in sync.
| instance.instance_variable_set(:@client, temporary_client) | ||
| yield | ||
| instance.instance_variable_set(:@client, before_client) | ||
| end |
There was a problem hiding this comment.
Very glad to see this method go - thank you for improving the tests! ⭐
| relative_path: relative, | ||
| source_url: url, |
There was a problem hiding this comment.
Nitpick: I'd rename your relative and url variables to relative_path and source_url. Then the above two lines can be reduced to:
| relative_path: relative, | |
| source_url: url, | |
| relative_path:, | |
| source_url:, |
Should be easier to follow, as there are fewer keys/variable names to keep track of.
| def data_for_github_doc(doc_path, repo_name) | ||
| contents = File.read(doc_path) | ||
|
|
||
| path = Pathname.new(doc_path) |
There was a problem hiding this comment.
I think you'll need to add require "pathname" on line 3 to use this.
| docs_path = doc.path.sub("docs/", "").match(/(.+)\..+$/)[1] | ||
| filename = docs_path.split("/")[-1] | ||
| title = ExternalDoc.title(contents) || filename | ||
| def data_for_github_doc(doc_path, repo_name) |
There was a problem hiding this comment.
I think we could give this parameter a better name:
| def data_for_github_doc(doc_path, repo_name) | |
| def data_for_github_doc(absolute_path_to_doc, repo_name) |
| docs_path_with_extension = path.each_filename.to_a.drop_while { |f| f != "docs" }.drop(1).join("/") | ||
| docs_path_without_extension = docs_path_with_extension.chomp(".md") | ||
| relative = path.each_filename.to_a.drop_while { |f| f != "docs" }.join("/") | ||
|
|
||
| basename_without_extension = File.basename(doc_path, ".md") |
There was a problem hiding this comment.
Hmm, you only use docs_path_with_extension to form docs_path_without_extension. I think we can cut out the middle-man and set all the variables we need up front a bit more cleanly:
| docs_path_with_extension = path.each_filename.to_a.drop_while { |f| f != "docs" }.drop(1).join("/") | |
| docs_path_without_extension = docs_path_with_extension.chomp(".md") | |
| relative = path.each_filename.to_a.drop_while { |f| f != "docs" }.join("/") | |
| basename_without_extension = File.basename(doc_path, ".md") | |
| relative_path = Pathname.new(absolute_path_to_doc) | |
| .each_filename | |
| .to_a | |
| .drop_while { |f| f != "docs" } | |
| .join("/") | |
| docs_path_without_extension = relative_path.delete_prefix("docs/").chomp(".md") | |
| basename_without_extension = docs_path_without_extension.split("/").last |
NB there's also no need for therelative variable anymore, we've already defined relative_path above, which you can refer to on line 104.
3 participants
Docs folder now fetched with git clone and sparse checkout. Tests updated as well to mock local repo docs.