se-edu / addressbook-level3

:ab::three: Address Book sample application (Level 3)
https://se-education.org/addressbook-level3
MIT License
28 stars 430 forks source link

Edit absolute links in Developer Guide to be relative links #70

Closed mehak24k closed 3 years ago

mehak24k commented 3 years ago

Fixes #51

canihasreview[bot] commented 3 years ago

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

damithc commented 3 years ago

Thanks for the fix @mehak24k Can you submit an update as per the bot comment above and also provide a full commit message as per https://se-education.org/guides/conventions/git.html ?

codecov-io commented 3 years ago

Codecov Report

Merging #70 (49790bc) into master (fb67d1f) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #70   +/-   ##
=========================================
  Coverage     72.18%   72.18%           
  Complexity      401      401           
=========================================
  Files            70       70           
  Lines          1233     1233           
  Branches        126      126           
=========================================
  Hits            890      890           
  Misses          311      311           
  Partials         32       32           

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 fb67d1f...49790bc. Read the comment docs.

canihasreview[bot] commented 3 years ago

v1

@mehak24k submitted v1 for review.

(:books: Archive)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/70/1/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
mehak24k commented 3 years ago

Thanks for the fix @mehak24k Can you submit an update as per the bot comment above and also provide a full commit message as per https://se-education.org/guides/conventions/git.html ?

Thank you for the feedback, I have refined the commit message and submitted an update with the bot.

siangernlow commented 3 years ago

While the proposed changes are valid, the links break on the published GitHub Pages site due to the different domain name. As such, the links would have to be changed anyway once the repository is forked.

A potential solution would be to use link references as specified here, but this is also unviable as the concatenation of links is not supported. That means that the references would have to be absolute links anyway and would not solve the issue.

damithc commented 3 years ago

A potential solution would be to use link references as specified here, but this is also unviable as the concatenation of links is not supported. That means that the references would have to be absolute links anyway and would not solve the issue.

@siangernlow You might be right that they have to remain absolute links. If no one else can come up with a solution in the next few days, we can give up on this issue and close this PR.

siangernlow commented 3 years ago

@damithc I have found a potential solution that could be considered.

We can define a variable in the front matter of DeveloperGuide.md which stores the repo path. Subsequently, the absolute links would then reference this variable. As a result, when the project is forked, only the value stored by the variable would have to be changed. I've included some images below to illustrate this.

image Creating a variable repo to store the path to be referenced

image Referencing the variable

Referring to above, when the project is forked, only the path stored in repo would have to change.

damithc commented 3 years ago

@damithc I have found a potential solution that could be considered.

We can define a variable in the front matter of DeveloperGuide.md which stores the repo path. Subsequently, the absolute links would then reference this variable. As a result, when the project is forked, only the value stored by the variable would have to be changed. I've included some images below to illustrate this.

If this is a site-wide setting, should we use the method in here instead?

siangernlow commented 3 years ago

If this is a site-wide setting, should we use the method in here instead?

Yes, we could use that method. In the _config.yml file, there is already a repository variable which could be used for the purpose.

damithc commented 3 years ago

Yes, we could use that method. In the _config.yml file, there is already a repository variable which could be used for the purpose.

Ah, good. Do submit a PR if you manage to get it working.

siangernlow commented 3 years ago

Noted!

siangernlow commented 3 years ago

It appears that GitHub's preview does not parse Liquid tags which are required to reference the variables. This causes the preview on GitHub to have broken links. So I guess in the end absolute links are the way to go.

damithc commented 3 years ago

It appears that GitHub's preview does not parse Liquid tags which are required to reference the variables. This causes the preview on GitHub to have broken links. So I guess in the end absolute links are the way to go.

I see. You mentioned that the variable is already present in the config file. How is it used currently? Doesn't the same problem apply to those usages?

siangernlow commented 3 years ago

I believe that the variable is being used by Jekyll in HTML files (header.html) to generate the published site. In this case, Jekyll is able to parse the Liquid tags, so there are no problems for that. This is also the reason why the proposed solution works on the published site, but not in the preview on GitHub.

damithc commented 3 years ago

I believe that the variable is being used by Jekyll in HTML files (header.html) to generate the published site. In this case, Jekyll is able to parse the Liquid tags, so there are no problems for that. This is also the reason why the proposed solution works on the published site, but not in the preview on GitHub.

Got it. I guess no choice but to leave the absolute links as they are. Thanks for the investigation @siangernlow