Skip to content

Conversation

@peternewman
Copy link
Member

An alternative option to #1593 although without the required error checking too.

Needs testing on a broad range of OSes if possible.
Copy link
Member

@FloEdelmann FloEdelmann left a comment

Choose a reason for hiding this comment

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

This looks like a better alternative to #1592 now.

@peternewman
Copy link
Member Author

This looks like a better alternative to #1592 now.

Thanks @FloEdelmann, I think you mean #1593 though.

I think we still want the set -e/set +e bit and we ought to keep the escaping of backslashes from that too if it's needed.

@Keeper-of-the-Keys
Copy link

Please note that #1593 also resolves the backslash character properly something which the original sed did not (see ASCII art in #1541) iirc.

As far as whether or not to use a long line or multiple lines... My understanding is we're trying to keep the lint beast at bay without a blacklist, but @peternewman is correct that it does make debugging easier.

@peternewman
Copy link
Member Author

Please note that #1593 also resolves the backslash character properly something which the original sed did not (see ASCII art in #1541) iirc.

Agreed @Keeper-of-the-Keys . Do you want to pull this fix into #1593, or would you rather I take the rest of #1593 and pull that into this PR?

@Keeper-of-the-Keys
Copy link

Whatever works :)
I'll try to test this over the weekend.

@peternewman
Copy link
Member Author

I'll try to test this over the weekend.

Thanks, FYI aside from observation and compilation, I've been testing changes don't affect the output using this snippet:

#!/bin/sh

for file in */README.md; do
  echo "Generating $file";
  sh convert_README_to_header.sh `dirname $file` testmakereadme/orig/`dirname $file`.h
  sh convert_README_to_header.new.sh `dirname $file` testmakereadme/new/`dirname $file`.h
done
diff testmakereadme/orig/ testmakereadme/new/

So far it works fine on Ubuntu 16.04/sed 4.2.2 and Raspbian Buster/sed 4.7.

@Keeper-of-the-Keys
Copy link

Keeper-of-the-Keys commented Feb 9, 2020

@peternewman Finally had time to check this, it does not as I suspected take care of the backslash character.
However this is easily remedied add the following before and regex that adds any backslashes:
-e 's/\\\/\\\\\\\/g'

As far as the count goes it was arrived at through trial and error, the first can be either 3 or 4 bachslashes and the second 7 or 8 but not 6.

@peternewman
Copy link
Member Author

@peternewman Finally had time to check this, it does not as I suspected take care of the backslash character.
However this is easily remedied add the following before and regex that adds any backslashes:
-e 's/\\\/\\\\\\\/g'

As far as the count goes it was arrived at through trial and error, the first can be either 3 or 4 bachslashes and the second 7 or 8 but not 6.

Great, thanks for testing @Keeper-of-the-Keys . I might get this into 0.10 as a bug fix, and then we can have yours, with the set -e and the backslash stuff in master for your extra slashes in your new docs.

@peternewman
Copy link
Member Author

Great, thanks for testing @Keeper-of-the-Keys . I might get this into 0.10 as a bug fix

Of course I'm talking nonsense, this functionality isn't in 0.10 at all, buggy or otherwise! 🤦‍♂️

Can you update #1593 with the changes from here please @Keeper-of-the-Keys ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants