Hexstring converter to base64 #951

msamsel · 2017-09-21T11:02:30Z

What is the purpose of this pull request?

New feature

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

Unit tests
Manual tests

What changes did you make?

Add tools for converting hexstrings to base64.

Required by #662

mlewand · 2017-09-21T18:26:12Z

core/tools.js

+		 * @param {String} hexstring string contained only hexadecimal values, e.g. "0AB7"
+		 * @returns {String} base64 string
+		 */
+		hexstring2base64: function( hexstring ) {


When picking a name, check the surrounding names. Any other convert methods uses convertXyzToXyz so you're missing convert prefix and it should be "to" rather than "2".

Use consistent letter casing: hexString rather than hexstring.

mlewand · 2017-09-21T18:28:43Z

core/tools.js

@@ -1518,6 +1518,71 @@
 		},

 		/**
+		 * Converts hexstring to base64 coode.


This markdown does not get converted at all:

Take the time and set up the ckeditor-docs builder, and preview any docs changes before pushing the code.

Reason for the above is probably no new line before the ordered list.

mlewand · 2017-09-21T18:41:14Z

core/tools.js

+		 * 	decimal: 2 10 4 29 34 45 40 43 -> base64: "CKEditor"
+		 *
+		 * @since 4.8.0
+		 * @param {String} hexstring string contained only hexadecimal values, e.g. "0AB7"


@param and @returns contain full sentences. So start with a capital letter and end with a dot.

mlewand · 2017-09-21T18:42:05Z

core/tools.js

+		 * 5. If there was added extra zeros bytes fill them with '=' sign.
+		 *
+		 * Example:
+		 * 	hex: 08A11D8ADA2B -> binary: 0000 1000 1010 0001 0001 1101 1000 1010 1101 1010 0010 1011


Don't be afraid to wrap code-related parts with back ticks, like so: 08A11D8ADA2B.

mlewand · 2017-09-21T18:43:52Z

tests/core/tools.js

+				},
+				{
+					hex: '08A11D8ADA2B',
+					base64: 'CKEditor'


mlewand · 2017-09-21T18:44:30Z

@msamsel and please, don't put these dots in a pull request title :D

mlewand · 2017-09-22T12:57:37Z

core/tools.js

+		 * @param {String} hexString String contained only hexadecimal values, e.g. `0AB7`.
+		 * @returns {String} Base64 string represnet input value.
+		 */
+		convertHexstringToBase64: function( hexString ) {


There's one more thing, I thought that I have included it in original review, but turns out I forgot to paste it in review summary.

Rather than adding convertHexstringToBase64 which does the job for your particular purpose, you should do two separate function:

convertHexStringToBytes

convertBytesToBase64

That way this code will be more reusable, and can be used by a developer that looks only to use bytes to base64 conversion, etc.

f1ames

One tiny detail to fix and we are ready to go.

f1ames · 2017-09-26T08:30:19Z

core/tools.js

+		 *
+		 * @since 4.8.0
+		 * @param bytesArray Array which stores 1 byte in each cell as Integer number.
+		 * @returns Base64 string which represents input bytes.


There is no such type as bytesArray or Base64, it should probably be Array and String. You can include more information (that it is array of bytes / string in base64 format) in the @param/@returns description. Also remember about brackets when defining type - {Array}.

mlewand

Two minor changes and we're ready to go.

mlewand · 2017-09-26T13:50:17Z

core/tools.js

+		/**
+		 * Convert bytes array into base64 code.
+		 *
+		 * Bytes are `8bit` numbers, where base64 use `6bit` to store data. That's why we process 3 Bytes into 4 characters representing base64.


Is this information in any way needed for the person using this API? I think not - this is an implementation detail of base64 and are not relevant for the person looking for this method.

Same goes for the "Algorithm" part. It's great that you have added it, but you did simply in a wrong place.

You don't want to put all of this information in our API docs, as there should be just summary. Instead it should be added as a inline comment insde the function, so that only programmers who work on this internals know it.

So I'll ask you to just do that, and we'll keep this great value of having these comments. 🙂

Agreed, first of all - kudos for explaining this in details, great job.

However, I'd recommend moving the detailed explanation of the algorithm to (internal) comments and leaving the public API docs more straightforward/basic. A nice example how we usually do it is the Magicline plugin with excellent comments by @oleq: https://github.com/ckeditor/ckeditor-dev/blob/major/plugins/magicline/plugin.js#L851

@AnnaTomanek 😊

Credit where it's due! Learn from the master, guys! :)

mlewand · 2017-09-26T13:54:03Z

tests/core/tools.js

+				}
+			];
+			CKEDITOR.tools.array.forEach( testCases, function( test ) {
+				assert.isTrue( CKEDITOR.tools.arrayCompare( test.bytes, CKEDITOR.tools.convertHexStringToBytes( test.hex ) ) );


There are dedicated assertions for array assertion (arrayAssert), see arrayAssert.itemsAreEqual.

mlewand

Added one commit with API docs adjustments. LGTM.

f1ames

LGTM 👍

AnnaTomanek · 2017-09-27T09:28:30Z

core/tools.js

+		 *
+		 * @since 4.8.0
+		 * @param {Array} bytesArray An array which stores 1 byte in each cell as Integer number.
+		 * @returns {String} Base64 string which represents input bytes.


Base64-encoded string.

msamsel changed the base branch from major to t/662 September 21, 2017 11:10

mlewand self-requested a review September 21, 2017 12:40

Add converter hexstring to base64 and unit tests to it.

558a687

msamsel force-pushed the t/662-2972 branch from d365cb7 to 558a687 Compare September 21, 2017 13:30

mlewand suggested changes Sep 21, 2017

View reviewed changes

msamsel changed the title ~~Hexstring converter to base64.~~ Hexstring converter to base64 Sep 22, 2017

Review fix: rename method and typo fixes.

701aaf3

msamsel force-pushed the t/662-2972 branch from 46226f8 to 701aaf3 Compare September 22, 2017 08:43

mlewand suggested changes Sep 22, 2017

View reviewed changes

Review: break hexstring convertion into 2 separate tools methods.

97082cd

msamsel force-pushed the t/662-2972 branch from e9b1565 to 97082cd Compare September 25, 2017 09:52

f1ames self-requested a review September 26, 2017 08:09

Some rewording.

f8f78a2

f1ames suggested changes Sep 26, 2017

View reviewed changes

Review: jsduck description improvement.

683a64d

mlewand self-requested a review September 26, 2017 13:11

mlewand suggested changes Sep 26, 2017

View reviewed changes

Mateusz Samsel and others added 2 commits September 26, 2017 16:11

Review: change assertion method, and move comments for internal purpose.

7eb406e

Minor API docs corrections.

25d262a

mlewand self-requested a review September 26, 2017 14:28

mlewand approved these changes Sep 26, 2017

View reviewed changes

mlewand requested a review from f1ames September 26, 2017 14:30

f1ames approved these changes Sep 26, 2017

View reviewed changes

f1ames merged commit 3ddfe4c into t/662 Sep 26, 2017

f1ames deleted the t/662-2972 branch September 26, 2017 16:06

AnnaTomanek reviewed Sep 27, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hexstring converter to base64 #951

Hexstring converter to base64 #951

msamsel commented Sep 21, 2017 •

edited

Loading

mlewand Sep 21, 2017

mlewand Sep 21, 2017

mlewand Sep 21, 2017

mlewand Sep 21, 2017

mlewand Sep 21, 2017

mlewand Sep 21, 2017

mlewand Sep 21, 2017

mlewand commented Sep 21, 2017

mlewand Sep 22, 2017

f1ames left a comment

f1ames Sep 26, 2017

mlewand left a comment

mlewand Sep 26, 2017

AnnaTomanek Sep 27, 2017

oleq Sep 27, 2017

AnnaTomanek Sep 27, 2017

mlewand Sep 26, 2017

mlewand left a comment

f1ames left a comment

AnnaTomanek Sep 27, 2017

Hexstring converter to base64 #951

Hexstring converter to base64 #951

Conversation

msamsel commented Sep 21, 2017 • edited Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

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

mlewand commented Sep 21, 2017

Choose a reason for hiding this comment

f1ames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlewand 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

mlewand left a comment

Choose a reason for hiding this comment

f1ames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msamsel commented Sep 21, 2017 •

edited

Loading