Update rebase-helper docs #271

ferdnyc · 2018-07-13T12:59:03Z

Did some cleanup, lots of formatting, and some minor copyediting in rebase-helper.md in the wiki:

Indented code blocks that are placed under bullet headings
Fenced code blocks with ```sh and ```python3 to enable (minimal) syntax highlighting
Fenced more filenames / shell commands / package identifiers with backticks
Removed backtick fencing from some headings and other non-code-literal words in favor of bolding or other (non-monospaced) decoration.
Some minor grammatical cleanup (on the section headings, primarily).

Did some cleanup, lots of formatting, and some minor copyediting in `rebase-helper.md` in the wiki: * Indented code blocks that are placed under bullet headings * Fenced code blocks with ````sh` and ````python3` to enable syntax highlighting * Fenced more filenames / shell commands / package identifiers with backticks, removed backtick fencing from some headings and other non-code-literal words in favor of bolding or other (non-monospaced) decoration. * Some grammatical cleanup on the section headings, especially

The syntax highlighting was also very useful as it highlighted several typos in the API examples. This fixes some more of them, as best I could _guess_ what they were supposed to be. But there are still some issues, since (for example) `fedpkg` isn't a legal argument to `--buildtool`. (The F28 `rebase-helper` says: "`ERROR: argument --buildtool: invalid choice: 'fedpkg' (choose from 'copr', 'koji', 'mock', 'rpmbuild')`".) But I only corrected what I could be reasonably sure of.

pvalena · 2018-08-21T07:20:02Z

Thanks for the PR!

pvalena

Additionally, is the code here tested? Does it apply to Fedora latest stable version?

pvalena · 2018-08-21T07:20:36Z

deployment/maintain/rebase-helper.md

@@ -8,10 +8,10 @@ order: 2
 Rebase-helper is a tool which helps package maintainers with updating package to the latest upstream version.
 It automates a lot of manual tasks the package maintainer usually does, when a new upstream version of a package is released.

-## How to install rebase-helper
+## Installing rebase-helper


I'd preffer Install instead of Installing.

Fair enough.

pvalena · 2018-08-21T07:22:50Z

deployment/maintain/rebase-helper.md

 Rebase-helper workflow can be summarized in following steps:

 - Downloads an archive with new sources.
 - Rebases downstream patches on top of the latest upstream version using git rebase command.
- If patching was successful, it builds two sets of RPMs. One using the ``old`` sources and a second one using the ``new`` sources. Available build tools are ``mock``, ``rpmbuild``, ``fedpkg``.
-  - If ``new`` RPMs build fails, downloads logs like build.log and root.log
+- If patching was successful, it builds two sets of RPMs. One using the **``old``** sources and a second one using the **``new``** sources. Available build tools are ``mock``, ``rpmbuild``, ``fedpkg``.


Regarding the markdown changes, I wouldn't put both ``
and ** there. Choose one, please.

Yeah, I never really understood why those strings were monospace-fenced to begin with (and only sometimes). I added the bolding to (consistently) make them stand out a bit, but stopped short of arbitrarily dropping the (equally-arbitrary) fixed-width formatting. I'll go the rest of the way.

Well, AFAICT it's because of the syntax hilighting of key words, see resulting page:

https://developer.fedoraproject.org/deployment/maintain/rebase-helper.html

Thinking about it some more, maybe we could live the backtics instead

That's the odd thing, though. old and new aren't actually keywords, as far as I can tell. They're closer to argument placeholders, in fact I'm wondering if they shouldn't be italicized instead.

pvalena · 2018-08-21T07:23:41Z

deployment/maintain/rebase-helper.md

+   rh = Application(cli)
+   rh.set_upstream_monitoring() # Switch rebase-helper to upstream release monitoring mode.
+   rh.run()
+   ```


This has unnecessary indent, right?

Oh, not at all, IMHO. "Python API" and "BASH" are two items in a bulleted list, the indent places the examples at the same level (inside their corresponding bullet points).

Well, even in case that is reflected on the resulting page properly, I'd prefer using (sub-)sub-headlines instead of those bullet points.

Mmm, actually in moving that part to a separate page I ended up raising everything a heading level, so now they will just be sub-sub. (Previously, the titles of the sectinons they occupy were already sub-sub. Now they're just subheads.) ...Done.

pvalena · 2018-08-21T07:25:00Z

deployment/maintain/rebase-helper.md


-* ``Python API``
+* **Python API**


It looks like there's a lot of directions in one page. Would you mind splitting it into multiple pages?

We've kind of moved beyond "directions" by this point in the document, TBH.

Everything from "Integrating rebase-helper with an upstream monitoring service" down is a separate topic. I think it's fine to be its own separate page, but it's also not really "directions". It's more a rough sketch of possible pseudocode that might be employed for that purpose.

You asked elsewhere if the code has been tested on current Fedora. As far as I know, this code never worked on any version of Fedora / fedpkg. I had nothing to do with drafting the original document, but my impression is that whoever wrote it just jotted down some half-formed thoughts and never really fleshed them out. One of my original commits included this note:

The syntax highlighting was also very useful as it highlighted several typos in the API examples. This fixes some more of them, as best I could guess what they were supposed to be. But there are still some issues, since (for example) fedpkg isn't a legal argument to --buildtool. (The F28 rebase-helper says: "ERROR: argument --buildtool: invalid choice: 'fedpkg' (choose from 'copr', 'koji', 'mock', 'rpmbuild')".) But I only corrected what I could be reasonably sure of.

I would be very surprised if any of the code in these examples was 100% usable as-is. But, while I can correct the formatting, I can't correct the code. I didn't write it.

I'll part out the "Integrating..." bit into its own page, though.

Thanks.
So fixing the code should be on issues list.
Every code snippet on this web should work (at least at some point), otherwise it's useless.

pvalena · 2018-08-21T07:25:29Z

deployment/maintain/rebase-helper.md


-* ``BASH``
+* **BASH**


Wouldn't command line be more accurate here?

I don't... think so. My impression is that the examples there were meant for /bin/sh shell scripting, when integrating into a monitoring service controlled by shell scripts. They weren't envisioned as interactive commands. But, that's purely my guess of the original author's intent, based on the context.

(In fact, while I added $ prompts to the start of these commands, too, I'm now thinking that may have been a mistake, for exactly those reasons.)

pvalena · 2018-08-21T07:27:15Z

deployment/maintain/rebase-helper.md

 Begin installation on Fedora using the ``dnf`` command:

-```
+```sh
 sudo dnf install -y rebase-helper


Additionally, would you mind removing the -y argument here?

Agreed, those always made me uncomfortable.

pvalena · 2018-08-21T07:30:14Z

deployment/maintain/rebase-helper.md


-```
+# Update to the selected upstream version
 rebase-helper 1.2.1


Actually, would you mind prefixing these commands with $?

I played with that at one point, and determined that the syntax highlighting really didn't handle that in a way that felt very readable. And it has the cost of making it a lot harder to cut-and-paste the commands, if someone wants to use them directly out of the document. I'll make the change, but IMHO of these two formats:

# Change to the location of foobar.spec and other package components (cloned dist-git dir), e.g. $ cd $HOME/rpmbuild/REPOS/foobar # Update to the selected upstream version $ rebase-helper 1.2.1

# Change to the location of foobar.spec and other package components (cloned dist-git dir), e.g. cd $HOME/rpmbuild/REPOS/foobar # Update to the selected upstream version rebase-helper 1.2.1

...The second is actually quite a bit clearer and more useful. (Again, purely IMHO.)

There's also console syntax highlighting, which is meant for console typescripts... but it doesn't style comments at all, and still fails to style the shell prompt usefully. Really it's more geared towards showing output along with input, which we aren't doing.

# Change to the location of foobar.spec and other package components (cloned dist-git dir), e.g. $ cd $HOME/rpmbuild/REPOS/foobar # Update to the selected upstream version $ rebase-helper 1.2.1

...So, no real wins there, but a few losses.

If we're to enhance the readability we'll do that site-wide. Feel free to submit an ehnancement (preferrably CSS or similar). As of now, we prefix all commands with $ (see our Contributing Guidelines), so please do include it.

Also, FYI: the jekyl-built website looks different compared to Githubs-Markdown display.

* Create new page `rebase-helper-integration.md` * Shift `static-analysis.md` to `order: 4` (from `order: 3`) * Extend header metadata based on example in `about.md`

pvalena · 2018-08-21T11:00:52Z

This branch has no conflicts with the base branch when rebasing

pvalena · 2018-08-21T11:11:04Z

I think you're right. Everything belonging to Integrating with upstream monitoring service is meant as a script snippet.
It's just an example though, and don't think anyone ever used it. Feel free to change it any way you wish.

Based on http://developer-portal.github.io/doc/file-structure.html

ferdnyc · 2018-08-21T11:15:31Z

I think I've now addressed everything we discussed, let me know if any more changes are needed. Thanks for taking the time to go through it!

pvalena · 2018-08-22T18:04:18Z

Looks great, thanks!

ferdnyc added 3 commits July 13, 2018 08:57

Missed one set of cmdline arg fixes

28b81bd

pvalena requested changes Aug 21, 2018

View reviewed changes

pvalena reviewed Aug 21, 2018

View reviewed changes

ferdnyc added 4 commits August 21, 2018 06:20

Don't use -y flag with dnf

cdf6832

Copyedits as requested

f56361a

Prefix shell commands with $ prompt

d352d6a

Split integration section out

8055a3d

* Create new page `rebase-helper-integration.md` * Shift `static-analysis.md` to `order: 4` (from `order: 3`) * Extend header metadata based on example in `about.md`

Promote example bullets to subsubheads

876e16f

ferdnyc added 2 commits August 21, 2018 07:11

Fix some YAML structural issues

fe8282f

Based on http://developer-portal.github.io/doc/file-structure.html

Drop $ prompts from script snippets

4a8a3d0

pvalena self-assigned this Aug 22, 2018

pvalena added Section: Language & Databases Status: In Progress Section: Deploy and Distribute and removed Section: Language & Databases labels Aug 22, 2018

pvalena merged commit ff2c69e into developer-portal:master Aug 22, 2018

pvalena mentioned this pull request Aug 22, 2018

Fix rebase-helper snippet examples #278

Open

ferdnyc deleted the patch-2 branch August 24, 2018 06:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update rebase-helper docs #271

Update rebase-helper docs #271

ferdnyc commented Jul 13, 2018

pvalena commented Aug 21, 2018

pvalena left a comment

pvalena Aug 21, 2018

ferdnyc Aug 21, 2018

pvalena Aug 21, 2018

ferdnyc Aug 21, 2018

pvalena Aug 21, 2018

ferdnyc Aug 21, 2018

pvalena Aug 21, 2018

ferdnyc Aug 21, 2018

pvalena Aug 21, 2018

ferdnyc Aug 21, 2018

pvalena Aug 21, 2018

ferdnyc Aug 21, 2018

pvalena Aug 21, 2018

pvalena Aug 21, 2018

ferdnyc Aug 21, 2018

ferdnyc Aug 21, 2018

pvalena Aug 21, 2018

ferdnyc Aug 21, 2018

This comment was marked as outdated.

pvalena Aug 21, 2018

ferdnyc Aug 21, 2018

pvalena Aug 21, 2018

pvalena commented Aug 21, 2018

pvalena commented Aug 21, 2018

ferdnyc commented Aug 21, 2018

pvalena commented Aug 22, 2018

Update rebase-helper docs #271

Update rebase-helper docs #271

Conversation

ferdnyc commented Jul 13, 2018

pvalena commented Aug 21, 2018

pvalena left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvalena commented Aug 21, 2018

pvalena commented Aug 21, 2018

ferdnyc commented Aug 21, 2018

pvalena commented Aug 22, 2018