ampproject
✨ Use placeholders for imported documents
This PR moves all imported documents in to a common directory pages/shared/imports which is ignored by version control. The files are then embedded into the actual documents by using something like [include('shared/imports/community/governance.md')].
By this doing an import doesn't cause changes to the working copy anymore which fixes #2442.
Adding @pbakaus for his insights in regards to translation.
I'm not convinced that this is the right direction:
- Maintaining placeholder files feels like a redundancy that we should avoid.
- The reason to keep the imported files in the repo was to make it easier for translators. This is no longer the case.
Hm. The redundancy is needed to have actual documents in the correct collections so Grow can render them in the navigation sidebars. But as their automatically created when the files are getting imported there's no additional maintenance to before.
Having those placeholder documents also is what enables translations similar to what we are doing with the component reference docs: one could now easily create a [email protected] besides the placeholder file (that just holds [include('shared/imports/community/governance.md')]) with the actual Spanish content maintained in this repository.
Love this PR! Thank you for doing this.
Regarding Sebastian's points, perhaps we can use symbolic links instead? This would address both of those concerns:
Maintaining placeholder files feels like a redundancy that we should avoid.
It's trivial to (re)generate symlinks: jq .[].to "platform/config/imports/spec.json" | xargs -I@ ln -sr "pages/shared/imports/@" "pages/content/amp-dev/@"
If we turn it into a git hook, we can probably automate it fully.
The reason to keep the imported files in the repo was to make it easier for translators. This is no longer the case.
From the translator's point of view, they could view the symbolically linked files easily and even modify them.
Symlinks would be an improvement and solve 2. I'm a bit scared of the additional complexity. But it's definitely an improvement over the status quo. We'd have to make sure that symlinks work cross platform though.
However, another thing I didn't realize earlier: having the placeholder files makes it possible for someone browsing the GitHub repo to find the original page (e.g. for fixing a spelling mistake via the GitHub UI). This wouldn't be possible and potentially very confusing with symlinks.
makes it possible for someone browsing the GitHub repo to find the original page
Not sure if this an argument for or against, but here's how a symlink behaves in GitHub UI:
https://github.com/google-pay/payment-data-cryptography-dotnet/blob/7463997f65aee631e8fecf5d6159a280162c259b/GooglePay.PaymentDataCryptography.Tests/testdata/ecdh_secp256r1_test.json
It shows the path to where it links to, so not too different from placeholders. But I can understand people not familiar with the concept of symlinking might be confused by it.
Symlinks add too much complexity imho.
I also don't fully understand how this much better than just gitignoring the files in place, but I could see the advantage for localization..
I currently don't have a very good recommendation on how to proceed here. Any input appreciated, thinking about it a little more over the weekend.
I also don't fully understand how this much better than just gitignoring the files in place, but I could see the advantage for localization..
Yup, this would have been the alternative. I refrained from doing so as you'd then need to maintain the imported documents in spec.json and .gitignore or (even worse) write the .gitignore file dynamically.
Update: My vote goes to gitignore generated files. After thinking about this a little longer, it just makes sense – generated files outside repo.
@pbakaus, for sure. But I assume not in the way this PR does it but instead add the all imported paths to the .gitignore manually instead, no?
@matthiasrohmer I'd say so, yeah. To unblock this, I'll let @sebastianbenz make the final call. There's pros and cons in all directions :)
I'd vote for Matthias's initial proposal then:
- manually maintaining files in
.gitignoreis hard (what happens if a guide file gets renamed) - it's impossible to discover the correct source file on Github
We only need to make sure that the grow build will fail if a placeholder does not match an imported file.
It is decided then!
On Thu, Aug 1, 2019, 5:23 PM Sebastian Benz [email protected] wrote:
I'd vote for Matthias's initial proposal then:
- manually maintaining files in .gitignore is hard (what happens if a guide file gets renamed)
- it's impossible to discover the correct source file on Github
We only need to make sure that the grow build will fail if a placeholder does not match an imported file.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ampproject/amp.dev/pull/2463?email_source=notifications&email_token=AAAKP7DHCY6D7E2CCZGPBI3QCKMPTA5CNFSM4IA4JLO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3JYW5Y#issuecomment-517180279, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAKP7GEPB45UZQK4AIZIQ3QCKMPTANCNFSM4IA4JLOQ .
@sebastianbenz, just resolved the conflicts. Can you explain what you mean by
need to make sure that the grow build will fail if a placeholder does not match an imported file.
Then I can look into this and the PR would be ready 🙂
@matthiasrohmer if a file is moved in the amphtml repo and no longer matches the placeholder, the build needs to fail.
@matthiasrohmer is this still something you think is worth pursuing?
@patrickkettner, thanks for flagging this again! Yeah I still think the approach implemented in this PR is a good solution for PRs polluted by imported files.
Though I am not quite sure if it still works with Grow 1. Worth noting: Sebastian's request is fulfilled. Grow fails for non-existing docs.