first cut at catching property name collisions during file output #273

chazzmoney · 2019-05-17T19:46:03Z

Bug found in #257, but not solving #257

Code to catch collisions in the array of properties immediately before file output.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

chazzmoney · 2019-05-17T19:46:47Z

This is NOT ready. Two issues:

I changed the basic example to cause errors so you can see what the output looks like
If y'all like it, I need to add tests

chazzmoney · 2019-05-17T19:47:13Z

@didoo and @dbanksdesign let me know what you think

dbanksdesign

Some minor things, overall I like it. 🙆‍♂️ What are your thoughts on either: moving those 50 lines in buildFile to a new file, or to a new function within that file for readability?

dbanksdesign · 2019-05-17T20:23:44Z

lib/utils/groupMessages.js

@@ -20,6 +20,7 @@ var GroupMessages = {
    RegisterTemplateDeprecationWarnings: 'Register Template Deprecation Warnings',
    SassMapFormatDeprecationWarnings: 'Sass Map Format Deprecation Warnings',
    MissingRegisterTransformErrors: 'Missing Register Transform Errors',
+    PropertyNameCollisionWarnings: 'Property Name Collision Warnings',


We are getting some real use out of the groupMessages! I never thought we'd have this many warnings and errors, but it follows our tenet to fail loudly! Super cool.

dbanksdesign · 2019-05-17T20:25:05Z

lib/buildFile.js

+
+  if(propertyNamesCollisionCount > 0) {
+    var propertyNamesCollisionWarnings = GroupMessages.flush(PROPERTY_NAME_COLLISION_WARNINGS).join('\n    ');
+    let title = `While building ${chalk.keyword(HIGHLIGHT_COLOR).bold(dest)}, token collisions were found; output may be unexpected.`;


"output may be unexpected" love it

dbanksdesign · 2019-05-17T20:25:51Z

lib/buildFile.js

+  console.log(chalk.bold.green((propertyNamesCollisionCount > 0) ? '⚠️ ' : '✔︎ ') + ' ' + destination);
+
+  if(propertyNamesCollisionCount > 0) {
+    var propertyNamesCollisionWarnings = GroupMessages.flush(PROPERTY_NAME_COLLISION_WARNINGS).join('\n    ');


Nit: you are using let in other places but var here, should we be consistent?

We should switch everything to ES6 as per #153 . I'm trying to stay aligned with the technique used in each file.

Crap, nm - using both in this file. I was moving things around a bit per our conversation and didn't change after a copy and paste. Will fix.

Ehhhhhhh.... so I just set all root level functional variables to use var and the block scoped variable to use let. Whatcha think?

I guess that is probably fine for now, and tackle it all in #153

dbanksdesign · 2019-05-17T20:27:18Z

lib/buildFile.js

+
+  // Check for property name Collisions
+  var nameCollisionObj = {};
+  filteredProperties.allProperties && filteredProperties.allProperties.forEach((propertyData) => {


Do we need this check? I think when we are constructing allProperties it will always be at least an empty array?

Unfortunately this is required. The tests do not generate allProperties at all in some cases, causing an error to throw here and the tests to fail.

dbanksdesign · 2019-05-17T20:28:21Z

lib/buildFile.js

+  var WARNING_COLOR = 'darkorange';
+  var HIGHLIGHT_COLOR = 'orangered';
+  var HELP_COLOR = 'orange';
+  Object.keys(nameCollisionObj).map((propertyName) => {


Change .map here to .forEach since we aren't actually mapping the array to some variable.

Whoops. Good call.

dbanksdesign · 2019-05-17T20:32:19Z

lib/buildFile.js

+  var HELP_COLOR = 'orange';
+  Object.keys(nameCollisionObj).map((propertyName) => {
+    if(nameCollisionObj[propertyName].length > 1) {
+      let collisions = nameCollisionObj[propertyName].map(properties => chalk.keyword(HIGHLIGHT_COLOR)(properties.path.join('.')) + '   ' + chalk.keyword(WARNING_COLOR)(properties.value)).join('\n        ');


This line is a bit long and hard to read. Maybe break this into a few lines?

dbanksdesign · 2019-05-17T20:35:58Z

lib/buildFile.js

+  if(propertyNamesCollisionCount > 0) {
+    var propertyNamesCollisionWarnings = GroupMessages.flush(PROPERTY_NAME_COLLISION_WARNINGS).join('\n    ');
+    let title = `While building ${chalk.keyword(HIGHLIGHT_COLOR).bold(dest)}, token collisions were found; output may be unexpected.`;
+    let help = chalk.keyword(HELP_COLOR)([


Maybe use back-ticks here since all you need the array for is to put new line characters in between each line. I like this message though. You could probably put title and help into a single string literal to make this cleaner.

I've had massive problems with formatting trying to use multiple backtick sections into each other. This was my solution.

I'm going to hold off on this change for now, and when we get everything else to a good place if you have a good suggestion maybe you can do a PR into this one - it was literally the piece that took me the longest.

Sounds good, after we merge this I can try to do a cleanup PR on this area.

dbanksdesign · 2019-05-17T20:37:58Z

lib/transform/config.js

@@ -29,6 +29,7 @@ var MISSING_TRANSFORM_ERRORS = GroupMessages.GROUP.MissingRegisterTransformError
 */
 function transformConfig(config, dictionary, platformName) {
  var to_ret = _.clone(config);
+  to_ret.platformName = platformName;


Is this being used elsewhere? I can't find this being used in this PR.

No it is not. I was using it to generate the message originally, but then realized I could just use the current output stream instead of generating something unique.

I thought it made sense to have the platform name present in the platform (that data is currently not available), so I left it. I can pull to a different PR if you want.

Yea let's pull it to a different PR, just so this one is focused on a single problem.

didoo · 2019-05-20T08:50:07Z

lib/buildFile.js

@@ -33,6 +34,8 @@ function buildFile(destination, format, platform, dictionary, filter) {
  if (!destination || typeof destination !== 'string')
    throw new Error('Please enter a valid destination');

+  var dest = destination;


probably I would not do this, but create a variable fullDestination (or something similar, I am not sure about the name) which is the one used and manipulated, and leave the value destination untouched, so it can be used directly in the construction of the title if propertyNamesCollisionCount > 0

didoo · 2019-05-20T08:53:24Z

lib/buildFile.js

+  });
+
+  var PROPERTY_NAME_COLLISION_WARNINGS = GroupMessages.GROUP.PropertyNameCollisionWarnings;
+  var WARNING_COLOR = 'darkorange';


I would not create variables just for this, at the end are just colors in Chalk, even if they are copy&pasted no big deal

If you guys like the current coloring, I can make this change. It was super painful changing colors when I was creating the output message so I used this to make it simpler.

If everyone confirms they like the output, I'll change this out.

changed to in-line

didoo · 2019-05-20T08:59:04Z

I have just added a couple of comments, but overall seems good to me.

dbanksdesign

LGTM!

chazzmoney · 2019-05-21T20:24:24Z

Sorry, was adding the tests still - and adding them required me to change something (the key for grouping messages is now per destination). Can you peek one more time? @dbanksdesign

dbanksdesign · 2019-05-21T21:29:12Z

Can you take a look at the test failure on travis?

chazzmoney · 2019-05-22T05:50:43Z

Well 💩. So apparently color coding in chalk differs on different platforms. Soooo... I dunno. I’ll come up with a way to rewrite the test.

dbanksdesign · 2019-05-22T18:00:37Z

You could use chalk methods in the test? Or how are we testing other console outputs that use chalk?

…acters

chazzmoney · 2019-05-23T05:51:29Z

Yeah, exactly what I did. Let me know if you still approve and we can MERGE BABY MERGE. 🤓

dbanksdesign

LGTM!

first cut at catching property name collisions during file output

3cf41fa

dbanksdesign reviewed May 17, 2019

View reviewed changes

didoo reviewed May 20, 2019

View reviewed changes

Charles Dorner added 4 commits May 20, 2019 10:47

multiple revisions based on PR comments

3827837

removing platformName change

15b9880

switching to in-line color names

54c5ada

reverting basic example back

66eb7e7

dbanksdesign approved these changes May 21, 2019

View reviewed changes

adding tests

8f3b412

fixed tests to check for output using chalk instead of hardcoded char…

4a2b8fa

…acters

dbanksdesign approved these changes May 24, 2019

View reviewed changes

dbanksdesign merged commit 9a40407 into master May 28, 2019

dbanksdesign deleted the catch-file-output-prop-name-collisions branch May 28, 2019 19:17

dbanksdesign mentioned this pull request Aug 12, 2019

What is the correct setup for sketch palette? #313

Closed

first cut at catching property name collisions during file output #273

first cut at catching property name collisions during file output #273

Conversation

chazzmoney commented May 17, 2019

chazzmoney commented May 17, 2019 • edited Loading

chazzmoney commented May 17, 2019

dbanksdesign 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

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

chazzmoney May 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

didoo commented May 20, 2019

dbanksdesign left a comment

Choose a reason for hiding this comment

chazzmoney commented May 21, 2019

dbanksdesign commented May 21, 2019

chazzmoney commented May 22, 2019

dbanksdesign commented May 22, 2019

chazzmoney commented May 23, 2019

dbanksdesign left a comment

Choose a reason for hiding this comment

chazzmoney commented May 17, 2019 •

edited

Loading

chazzmoney May 20, 2019 •

edited

Loading