WIP: Patch workflow for ckeditor #17

julien · 2019-05-09T13:29:59Z

@wincent, This is a first attempt at writing a patch workflow for ckeditor ... so please don't be afraid at what you're about to see 🙈😱

I thought adding this info in the README was the right thing to do.
There's probably an issue with the way the patches are generated but that's why I'm asking you for a review.
Maybe we could combine all three scripts into one and use ./scripts/patch.sh with an argument like: sh ./scripts/patch.sh init|create|build ?

Tell me what you think when you can.

Thanks!

EDIT: Once I'll have made the required changes for this to work properly, I'll apply the first patch

wincent

Maybe we could combine all three scripts into one and use ./scripts/patch.sh with an argument like: sh ./scripts/patch.sh init|create|build ?

This is a splendid idea. I'd say though it should be only two scripts/subcommands and not three. The "init" one should be an implicit thing that happens automatically if you haven't done it yet.

README.md

scripts/path.sh

scripts/build.sh

scripts/path.sh

scripts/update.sh

julien · 2019-05-10T11:55:39Z

@wincent, I'm happier with the result than I was yesterday but still need your help to review when you can.

Obviously, this is not foolproof (i.e. I can still delete the patches directory and try to run build which will fail at the moment) so I could probably improve a thing or two, but I think it will still be more useful than having to do everything manually.

Thanks again for the help!

julien · 2019-05-10T13:55:52Z

I could probably improve a thing or two

The build script should still work as before (allowing a $VERSION to be passed)
Update README.md
Update post in loop

Nice to have

More emojis?
Small .gifs or videos to show the "tool" in action

wincent

This is great. Especially the emoji. I've left some comments but I think you should feel free to merge this whenever you consider it ready; we can always add more commits later if we identify improvements or bugs.

README.md

wincent · 2019-05-13T07:17:39Z

README.md

+	sh ck.sh patch
+	```
+
+7. Create a build of CKEditor containing the patches:


In the rendered output, this is "6". You could update this (and the following one) to match, or leave it. Depends on whether you care more about minimizing effort or making the source Markdown as readable/correct as possible. For example, some people always just make every numbered list item "1." in Markdown.

That's interesting ... good point
As for the "fix" , I habe my doubts, I can always update it since this is easy enought, but it might be better to use bullet points? For example, in two months we might discover that there is a missing step and we'll have to "update" everything again.

Right, that's exactly why some people write numbered lists like this in Markdown:

1. first 1. second 1. third

In the words of the immortal(?) John Gruber himself:

The point is, if you want to, you can use ordinal numbers in your ordered Markdown lists, so that the numbers in your source match the numbers in your published HTML. But if you want to be lazy, you don’t have to.

I don't really know what the right answer is. In general, I like being lazy, but I always felt weird numbering things as "1.", "1.", "1." etc so I never did it.

I like being lazy too

README.md

ck.sh

wincent · 2019-05-13T07:18:48Z

ck.sh

+function message() {
+	if [[ ! -z "$1" ]] ; then
+		echo
+		printf -- '-%.0s' {1..80}; echo ""


wincent · 2019-05-13T07:33:43Z

ck.sh

+		# Save SHA1 for later
+		sha1=`git submodule | grep ckeditor-dev | awk '{print $1}' | sed -e s/[^0-9a-f]//`
+
+		# CD into the right place


This comment seems a bit redundant.

wincent · 2019-05-13T07:35:25Z

ck.sh

+		git checkout liferay --quiet
+
+		# Check for existing patches
+		if ls ../patches/* &>/dev/null; then


If you wanted to be stricter here you could look for files named *.patch.

Yes! I'll update that.

wincent · 2019-05-13T07:39:20Z

ck.sh

+			message "⚠️  WARNING" "$str"
+
+			echo
+			echo "$(ls ../patches/*)"


Isn't this the same as just?

ls ../patches/*

(Although I'd suggest ../patches/*.patch instead.)

Yes it is. I'll fix that.

wincent · 2019-05-13T07:39:56Z

ck.sh

+			echo
+
+			# Prompt the user to confirm he wants to delete existing patches
+			read -p "Are you sure you want to continue [y/n]? " yn


Again because init is destructive, we might want to do the prompting a bit earlier.

Ok yes seems better

wincent · 2019-05-13T07:40:44Z

ck.sh

+		usage
+		;;
+esac
+


Looks like an extra line.

Yes, correct

…review

wincent

Looks awesome!

wincent · 2019-05-14T07:13:16Z

ck.sh

+				echo "Don't forget to commit the result!"
+				echo
+				echo "git add -A -- ckeditor"
+				echo "git commit -m 'Update CKEDITOR'"


Just a minor nit; what would be an example of a good commit message here? If we can show one automatically, great; otherwise we should probably leave it off (your instruction to "commit the result" is probably enough). I just don't want the history to wind up with a bunch of messages that all say "Update CKEDITOR", because you'll have to look at the commits (which may be large) in order to figure out what that actually means.

julien

Looks awesome!

Thanks

I just don't want the history to wind up with a bunch of messages that all say "Update CKEDITOR", because you'll have to look at the commits (which may be large) in order to figure out what that actually means.

Yes that's a good point.

julien · 2019-05-13T14:11:08Z

ck.sh

+# Check arguments
+if [ $# -ne 1 ]; then
+	usage
+  exit 1


Yes, good catch

julien · 2019-05-13T14:30:16Z

README.md

+	sh ck.sh patch
+	```
+
+7. Create a build of CKEditor containing the patches:


julien · 2019-05-13T14:30:24Z

README.md

+	sh ck.sh patch
+	```
+
+7. Create a build of CKEditor containing the patches:


I like being lazy too

julien · 2019-05-13T14:30:41Z

ck.sh

+
+		if [[ ! -z  "$2" ]] ; then
+			echo
+			echo "$2" | fold -w 80


Here's my take on this: Since we're assuming that Windows users have git and all the wonderful tools that are required to use it, they will most probably be using Git for Windows, which comes with Git Bash (it's probably fancier than calling this msys2, mingw32 or however it is officially called) and it has all the *nix utilities like cat, find, fold, uniq ... that might change with Microsoft planning to ship a Linux kernel with Windows.

In anycase you're right, if we have to, we could re-write this in JavaScript and make use of our good old friend child_process.

julien · 2019-05-13T14:31:55Z

ck.sh

+	echo
+	echo "  🔧  setup: Setup everything to start working on a patch"
+	echo "  💉  patch: Generate patches "
+	echo "  🔥  build: Generate a patched version of CKEditor"


julien · 2019-05-13T14:33:11Z

ck.sh

+		git checkout liferay --quiet
+
+		# Check for existing patches
+		if ls ../patches/* &>/dev/null; then


Yes! I'll update that.

julien · 2019-05-13T14:33:15Z

ck.sh

+			message "⚠️  WARNING" "$str"
+
+			echo
+			echo "$(ls ../patches/*)"


Yes it is. I'll fix that.

julien · 2019-05-13T14:33:23Z

ck.sh

+		usage
+		;;
+esac
+


Yes, correct

julien · 2019-05-13T14:33:34Z

ck.sh

+			echo
+
+			# Prompt the user to confirm he wants to delete existing patches
+			read -p "Are you sure you want to continue [y/n]? " yn


Ok yes seems better

julien · 2019-05-13T14:35:41Z

ck.sh

+		if ! git rev-parse --verify liferay &>/dev/null; then
+			str=$(printf "%s"
+				"It seems that there's no *liferay* branch in the *ckeditor-dev* submodule.\n\n" \
+				"Please run *sh ./ck.sh setup* to set up.")


Well, after thinking more about this I actually think they key here is:

Again here, wouldn't just multiple echo calls be simpler and more readable?

And my answer would be

Yes, this message function is probably a good attempt at not repeating code, but it's way too #fancy and doesn't work with multiline input. So I'll use echo.

WIP: Patch workflow for ckeditor

adb1de7

wincent reviewed May 9, 2019

View reviewed changes

Julien Castelain added 2 commits May 10, 2019 13:49

chore: remove existing scripts

7c43204

feat: Add script to setup, patch and build CKEditor

125b7ce

chore: Update README

3749817

julien force-pushed the patch-workflow branch from c372ae3 to 3749817 Compare May 10, 2019 15:19

chore: Improve message printing

b8fb327

julien mentioned this pull request May 10, 2019

Define workflow for patching CKEditor #16

Closed

julien requested a review from wincent May 10, 2019 16:35

wincent approved these changes May 13, 2019

View reviewed changes

Julien Castelain added 2 commits May 13, 2019 16:50

chore: update README.md

d54c0dc

chore: remove the message function and make changes discussed during …

c0b2f02

…review

wincent approved these changes May 14, 2019

View reviewed changes

chore: Update message

5230a9e

julien commented May 14, 2019

View reviewed changes

julien merged commit b97685b into liferay:master May 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Patch workflow for ckeditor #17

WIP: Patch workflow for ckeditor #17

julien commented May 9, 2019 •

edited

Loading

wincent left a comment

julien commented May 10, 2019

julien commented May 10, 2019 •

edited

Loading

wincent left a comment

wincent May 13, 2019

julien May 13, 2019

wincent May 13, 2019

julien May 13, 2019

julien May 13, 2019

wincent May 13, 2019

wincent May 13, 2019

julien May 13, 2019

wincent May 13, 2019

julien May 13, 2019

wincent May 13, 2019

julien May 13, 2019

wincent May 13, 2019

julien May 13, 2019

wincent May 13, 2019

julien May 13, 2019

wincent left a comment

wincent May 14, 2019 •

edited

Loading

julien left a comment

julien May 13, 2019

julien May 13, 2019

julien May 13, 2019

julien May 13, 2019

julien May 13, 2019

julien May 13, 2019

julien May 13, 2019

julien May 13, 2019

julien May 13, 2019

julien May 13, 2019

WIP: Patch workflow for ckeditor #17

WIP: Patch workflow for ckeditor #17

Conversation

julien commented May 9, 2019 • edited Loading

wincent left a comment

Choose a reason for hiding this comment

julien commented May 10, 2019

julien commented May 10, 2019 • edited Loading

wincent 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

wincent left a comment

Choose a reason for hiding this comment

wincent May 14, 2019 • edited Loading

Choose a reason for hiding this comment

julien 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

julien commented May 9, 2019 •

edited

Loading

julien commented May 10, 2019 •

edited

Loading

wincent May 14, 2019 •

edited

Loading