Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update readme guidance #1359

Merged
merged 2 commits into from
Jun 1, 2020
Merged

update readme guidance #1359

merged 2 commits into from
Jun 1, 2020

Conversation

KarishmaGhiya
Copy link
Member

No description provided.

@weshaggard
Copy link
Member

PTAL @maggiepint @scbedd

@weshaggard
Copy link
Member

Addresses #1214.

@weshaggard
Copy link
Member

@heaths I believe you were also interested in reviewing the guidance for links.

@@ -22,7 +22,7 @@ Use the guidelines in each section of this template to ensure consistency and re
- [Samples](./samples)
Copy link
Member

Choose a reason for hiding this comment

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

@KarishmaGhiya can you update the links here to match the guidance.

Copy link
Member

Choose a reason for hiding this comment

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

As well in the sample readme next to this md.

Copy link
Member Author

Choose a reason for hiding this comment

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

The samples and changelog don't exist. Should we remove them from the bulleted list?

heaths
heaths previously requested changes May 28, 2020
@@ -22,7 +22,7 @@ Use the guidelines in each section of this template to ensure consistency and re
- [Samples](./samples)
- [Versioned API References](https://azure.github.io/azure-sdk-for-python/ref/Cosmos.html) -- Note this link format may be updated in the future but it is the best we have for now.

* **DO** use relative links to other files in the source repository. Relative links will be converted to absolute links using the commit sha reference during publishing, that will ensure that we don't break links when we move things around in the repo in the future. So avoid using absolute links to a branch like `master` as they will definitely be broken in the future.
* **DO NOT** use relative links to other files in the source repository. Use links to master in all our md files in the repo as this will help ensure that links work and don't break in a lot of cases. However when we publish these to docs we have the opportunity to transform those links to use the release tags instead of the master links.
Copy link
Member

Choose a reason for hiding this comment

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

If the links are being transformed when releasing, why should they be absolute? As relative links, they not only link to the same commit (i.e. same source version instead of something that could be very different), but they also work in local clones so people browsing the source locally can navigate the files. Can you explain the benefit in using fully-qualified URLs?

Copy link
Member

Choose a reason for hiding this comment

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

The biggest issue is that while we can and will transform the links in some workflows there are a number of other workflows that we don't have an opportunity to plug-in a step that will transform the links. I called out a couple examples in #1214.

Copy link
Member

Choose a reason for hiding this comment

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

But then what do we do about files out of sync? Viewing a file in one commit (perhaps online, or in a local clone) will potentially link to a different commit that may work differently? Since release tags are deterministic, at least for releases can we ask/require that people use those instead?

Copy link
Member

Choose a reason for hiding this comment

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

The guidance will definitely be to use release-tags for links when we control the pipeline for publishing them.

As for source links yes they do have the possibility to get out of sync but no matter what we do there will be some links that might get out of sync and if you are working in the scope of the repo you have a much better chance of finding and fixing links then folks in a completely different context.

@heaths heaths requested a review from weshaggard May 28, 2020 21:56
Copy link
Member

@weshaggard weshaggard left a comment

Choose a reason for hiding this comment

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

Please update the samples to match.

Copy link
Member

@scbedd scbedd left a comment

Choose a reason for hiding this comment

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

This leaves us in a much better state for all the automation. If someone manages to stumble upon the individual readme, links will at the very least now resolve.

EDIT: Do we plan to check in the handling of master links prior to merging this guidance update?

@weshaggard
Copy link
Member

Do we plan to check in the handling of master links prior to merging this guidance update?

I wanted us to get agreement on the guidance first and then @KarishmaGhiya is going to work on the implementation. She has already started on doing some of that work in parallel.

@heaths heaths dismissed their stale review June 1, 2020 19:13

I'll follow the guidance, but I don't think absolute URLs are the correct solution. Hopefully we can improve upon this later on.

@KarishmaGhiya KarishmaGhiya merged commit 09b6c8e into master Jun 1, 2020
@KarishmaGhiya KarishmaGhiya deleted the readmeUpdate branch June 1, 2020 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants