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

cronjob: fix red new version box injection and script documentation #1173

Merged
merged 6 commits into from
Aug 29, 2024

Conversation

neteler
Copy link
Member

@neteler neteler commented Aug 15, 2024

This PR modifies the way to inject the "Note: This document is for an older version of GRASS GIS ..." red boxes into the manual page. Now duplicate boxes should no longer show up.

(affects G78 and G83 core manual and related addon manual pages)

For testing purposes, this PR has been deployed on grass.osgeo.org (once this PR is merged the local update needs to be removed from server to re-enable cronjob-based GH pulls, tbd by MN).

Hint: Best is to check how the pages look like not before Aug 17th, 2024, in order to have several daily cronjob cycles done.

This PR modifies the way to inject the "Note: This document is for an older version of GRASS GIS ..." red boxes into the manual page.
Now duplicate boxes should no longer show up.

(affects G78 and G83 core manual and related addon manual pages)
@neteler neteler added bug Something isn't working CI Continuous integration labels Aug 15, 2024
@neteler neteler self-assigned this Aug 15, 2024
@veroandreo
Copy link
Contributor

Was this always the case to have the red box in the footer too?

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

Can you please add a line or two describing what the commands are doing. Right now, the code is heavily write-only.

@neteler
Copy link
Member Author

neteler commented Aug 15, 2024

Was this always the case to have the red box in the footer too?

As I said: Best is to check how the pages look like not before Aug 17th, 2024, in order to have several daily cronjob cycles done.

@veroandreo
Copy link
Contributor

Was this always the case to have the red box in the footer too?

As I said: Best is to check how the pages look like not before Aug 17th, 2024, in order to have several daily cronjob cycles done.

Oh, ok, I understood that the PR should be checked before that date... sorry for the noise then 🙏

@neteler
Copy link
Member Author

neteler commented Aug 16, 2024

Was this always the case to have the red box in the footer too?

No, it wasn't. In the overview pages the box was entirely missing.

Although I was surprised by the red box in the footer, on second thought it might not be so bad, especially for long pages. What do you think?

@veroandreo
Copy link
Contributor

Was this always the case to have the red box in the footer too?

No, it wasn't. In the overview pages the box was entirely missing.

Although I was surprised by the red box in the footer, on second thought it might not be so bad, especially for long pages. What do you think?

I believe the one in the header is enough, no? That's the first thing one sees when opening a new page, and should be enough warning.

@echoix
Copy link
Member

echoix commented Aug 17, 2024

I saw some pages that had the header, but I stumbled upon one that only has a footer: https://grass.osgeo.org/grass83/manuals/variables.html

image
image

A page with header and footer: https://grass.osgeo.org/grass83/manuals/d.graph.html
image
image

@neteler
Copy link
Member Author

neteler commented Aug 25, 2024

Please re-check:

I have uploaded a fix to the server. If ok, I'll submit it here and update this PR accordingly.

@veroandreo
Copy link
Contributor

Please re-check:

I have uploaded a fix to the server. If ok, I'll submit it here and update this PR accordingly.

looking good now!

@neteler
Copy link
Member Author

neteler commented Aug 27, 2024

Now also fixed (fix uploaded to the server for testing):

@neteler
Copy link
Member Author

neteler commented Aug 27, 2024

Can you please add a line or two describing what the commands are doing. Right now, the code is heavily write-only.

Sure: improved documentation of scripts in

@neteler neteler changed the title cronjob: fix red new version box injection cronjob: fix red new version box injection and script documentation Aug 27, 2024
@neteler
Copy link
Member Author

neteler commented Aug 29, 2024

"Merging is blocked" - I'd like to move on here.

Is anything still missing?

@echoix
Copy link
Member

echoix commented Aug 29, 2024

"Merging is blocked" - I'd like to move on here.

Is anything still missing?

A requested changes by @wenzeslaus is still applied.

@wenzeslaus wenzeslaus removed their request for review August 29, 2024 12:42
@neteler
Copy link
Member Author

neteler commented Aug 29, 2024

A requested changes by @wenzeslaus is still applied.

I seem to need a hint.

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

I agree with @echoix. I was indeed talking about documentation for the changed and complex lines. But I'm ready to move on here.

@wenzeslaus wenzeslaus dismissed their stale review August 29, 2024 12:52

It is more beneficial just to move on.

@neteler
Copy link
Member Author

neteler commented Aug 29, 2024

A requested changes by @wenzeslaus is still applied.

Ok, see 1668170

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

Thanks!

@neteler neteler merged commit 428c2f7 into OSGeo:grass8 Aug 29, 2024
7 checks passed
@neteler neteler deleted the cronjob_fix_version_box branch August 29, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CI Continuous integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants