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

report_results.csh: add html anchors to hashes and machines #290

Merged
merged 1 commit into from
Dec 19, 2019

Conversation

phil-blain
Copy link
Member

@phil-blain phil-blain commented Dec 9, 2019

PR checklist

  • Short (1 sentence) summary of your PR:
    Add html anchors to hashes and machines in Test-Results wiki
  • Developer(s):
    P. Blain
  • Suggest PR reviewers from list in the column to the right.@apcraig
  • Please copy the PR test results link or provide a summary of testing completed below.
    None
  • How much do the PR code changes differ from the unmodified code?
    • bit for bit
    • different at roundoff level
    • more substantial
  • Does this PR create or have dependencies on CICE or any other models?
    • Yes
    • No
  • Does this PR add any new test cases?
    • Yes
    • No
  • Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/.)
    • Yes
    • No, does the documentation need to be updated at a later time?
      • Yes
      • No
  • Please provide any additional information or relevant details below:

The hashes and machine names inserted by the script in the different pages
of the Test-Results wiki are formatted as bold (** ${hash} **:).

Change this to level 4 HTML titles (<h4>) so that an HTML anchor is
automatically created, which makes it easier to link directly to specific
hash from a pull request.

@phil-blain
Copy link
Member Author

xref: CICE PR with the same changes: CICE-Consortium/CICE#388

Copy link
Contributor

@apcraig apcraig left a comment

Choose a reason for hiding this comment

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

I tested this and found a couple problems. See https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks hash #89af3098a. First, no link is really created and I'm still unclear what the purpose of this is. There is a link symbol, but it doesn't seem to do anything. Second, the results are not being grouped by hash anymore. I think the "chk" logic two lines above the "Note" needs to be modified to not check for "**${hash}" but to check for "#### ${hash}" for both by hash and by machine. Third, for the machines list, this will not be fully backwards compatible in the sense that it will create new machines tables. I think that's ok, but think it's worth pointing out. I have not tested the CICE version of this PR, but assume it also needs the same changes.

I think the main thing is that the chk lines need to be fixed. Other than that, I'm still unclear what purpose this serves. We still have to go in manually later and add the actual links?

@phil-blain
Copy link
Member Author

Thanks for the review :)

First, no link is really created and I'm still unclear what the purpose of this is.

The purpose of this PR is to create HTML anchors (the #anchor part at the end of a URL) on the pages of the Test-Results wiki, not links. This means that when we reference the result of a test in a related PR, we can link directly to the test hash instead of just mentioning it. So instead of writing as you did above:

See https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks hash #89af3098a.

We can simply link directly to the hash:
"See https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#89af3098a892d595967c0a690c4a37ace616d7cf"

The anchor-including URL of a specific test can be obtained by hovering on the hash on the test result page and then clicking on the anchor icon that appears (but since the hash is the anchor, we can also type "https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks", type '#' and then paste the full hash from somewhere else if it's more convenient. Both work.

Second, the results are not being grouped by hash anymore. I think the "chk" logic two lines above the "Note" needs to be modified to not check for "**${hash}" but to check for "#### ${hash}" for both by hash and by machine.

That's true, I missed that. I will modify the grep argument and update both PRs. After that all results for the same hash should go in the same table.

Third, for the machines list, this will not be fully backwards compatible in the sense that it will create new machines tables.

That's also true. We could edit the machine pages manually, doing the same markup changes as in this PR (bold -> H4) and then any new test would not create new tables, as long as everybody posting results to the page updates their local repo(s) after the two PRs are merged.

@apcraig
Copy link
Contributor

apcraig commented Dec 18, 2019

Thanks @phil-blain, I understand the purpose the links serve now. It's not to point to something outside the results page, this makes it easier to point to a specific section ON the results page. I agree that's a nice feature, thanks for thinking of it and proposing it. Once the suggested fixes are put in place, I am happy to merge this. I'm not super worried about the backwards compatibility issue. Once we start to generate some test results with this new feature, I'll plan to manually update a few pages to support the transition, but it's not a critical issue. Thanks.

The hashes and machine names inserted by the script in the different pages
of the Test-Results wiki are formatted as bold (`** ${hash} **:`).

Change this to level 4 HTML titles (<h4>) so that an HTML anchor is
automatically created, which makes it easier to link directly to specific
hash from a pull request.
@phil-blain
Copy link
Member Author

I changed all instances of \*\* to ####
(I had forgotten 2 on the first force-push).

Copy link
Contributor

@apcraig apcraig left a comment

Choose a reason for hiding this comment

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

I tested the updates and all looks good. Thanks!

@apcraig apcraig merged commit 9c735fe into CICE-Consortium:master Dec 19, 2019
lettie-roach pushed a commit to lettie-roach/Icepack that referenced this pull request Oct 18, 2022
@phil-blain phil-blain deleted the tests-wiki-add-anchors branch February 13, 2024 16:21
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.

2 participants