Skip to content

Conversation

@valmynd
Copy link

@valmynd valmynd commented Nov 26, 2016

Hello,

I did a port to ES6, maybe you're interested in taking over the changes.
I'm sorry that I did a lot of reformatting, that makes up a bulk of the changes in the diffs...

The tests are running fine, I don't know if I should've added the dist-folder to the .gitignore-file?

Anyway, thanks for your awesome implementation of CIEDE2000!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.7%) to 100.0% when pulling cb809b8 on valmynd:master into ad7d2da on markusn:master.

@coveralls
Copy link

coveralls commented Nov 26, 2016

Coverage Status

Coverage increased (+0.7%) to 100.0% when pulling ebf22f1 on valmynd:master into ad7d2da on markusn:master.

@valmynd valmynd mentioned this pull request Nov 26, 2016
@markusn
Copy link
Owner

markusn commented Nov 28, 2016

Hi! Thanks for the PR, I will have a look at it when I get the time (this week is a bit busy). As you already stated the dist folder should not be part of the git-repo (it should be in the .gitignore file) as it is a build artifact (this should be fixed before the PR is accepted).

@coveralls
Copy link

coveralls commented Nov 28, 2016

Coverage Status

Coverage increased (+0.7%) to 100.0% when pulling 2b7b52e on valmynd:master into ad7d2da on markusn:master.

@markusn
Copy link
Owner

markusn commented Dec 1, 2016

I've tried to have a look at the diff, however it is very hard due to the fact that most of the changes is reformatting, shadowing the actual changes related to ES6. The reformatting should be removed, it makes it hard to review and does (in my opinion) not add any benefits.

The structure of the files also changed: the structure used in the source files should be divided into sections in the following order: imports/exports, followed by the definitions of exported functions, followed by internal functions. This structure is there for a good reason: when examining a source file it should be immediately obvious which imports and exports it has and after that the interesting (public) functions should be defined (followed by the uninteresting internal functions).

@markusn markusn closed this Apr 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants