Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
May 28, 2019
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3cf41fa
first cut at catching property name collisions during file output
May 17, 2019
3827837
multiple revisions based on PR comments
May 20, 2019
15b9880
removing platformName change
May 21, 2019
54c5ada
switching to in-line color names
May 21, 2019
66eb7e7
reverting basic example back
May 21, 2019
8f3b412
adding tests
May 21, 2019
4a2b8fa
fixed tests to check for output using chalk instead of hardcoded char…
May 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions examples/basic/properties/color/base.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
"base": {
"gray": {
"light" : { "value": "#CCCCCC" },
"Light" : { "value": "#DDD" },
"medium": { "value": "#999999" },
"Medium": { "value": "#aaaaaa" },
"dark" : { "value": "#111111" }
},
"red": { "value": "#FF0000" },
Expand Down
50 changes: 47 additions & 3 deletions lib/buildFile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
var path = require('path'),
fs = require('fs-extra'),
chalk = require('chalk'),
filterProperties = require('./filterProperties');
filterProperties = require('./filterProperties'),
GroupMessages = require('./utils/groupMessages');

/**
* Takes the style property object and a format and returns a
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agreed


// if there is a build path, prepend the destination with it
if (platform.buildPath) {
destination = platform.buildPath + destination;
Expand All @@ -42,8 +45,49 @@ function buildFile(destination, format, platform, dictionary, filter) {
if (!fs.existsSync(dirname))
fs.mkdirsSync(dirname);

fs.writeFileSync(destination, format(filterProperties(dictionary, filter), platform));
console.log(chalk.bold.green('✔︎') + ' ' + destination);
let filteredProperties = filterProperties(dictionary, filter);

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

let propertyName = propertyData.name;
if(!nameCollisionObj[propertyName]) {
nameCollisionObj[propertyName] = [];
}
nameCollisionObj[propertyName].push(propertyData);
});

var PROPERTY_NAME_COLLISION_WARNINGS = GroupMessages.GROUP.PropertyNameCollisionWarnings;
var WARNING_COLOR = 'darkorange';
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@chazzmoney chazzmoney May 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed to in-line

var HIGHLIGHT_COLOR = 'orangered';
var HELP_COLOR = 'orange';
Object.keys(nameCollisionObj).map((propertyName) => {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whoops. Good call.

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 ');
Copy link
Member

Choose a reason for hiding this comment

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

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

GroupMessages.add(
PROPERTY_NAME_COLLISION_WARNINGS,
`Output name ${chalk.keyword(HIGHLIGHT_COLOR).bold(propertyName)} was generated by:\n ${collisions}`
);
}
});

let propertyNamesCollisionCount = GroupMessages.count(PROPERTY_NAME_COLLISION_WARNINGS);

fs.writeFileSync(destination, format(filteredProperties, platform));
console.log(chalk.bold.green((propertyNamesCollisionCount > 0) ? '⚠️ ' : '✔︎ ') + ' ' + destination);

if(propertyNamesCollisionCount > 0) {
var propertyNamesCollisionWarnings = GroupMessages.flush(PROPERTY_NAME_COLLISION_WARNINGS).join('\n ');
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

let title = `While building ${chalk.keyword(HIGHLIGHT_COLOR).bold(dest)}, token collisions were found; output may be unexpected.`;
Copy link
Member

Choose a reason for hiding this comment

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

"output may be unexpected" love it

let help = chalk.keyword(HELP_COLOR)([
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

'This many-to-one issue is usually caused by some combination of:',
'* conflicting or similar paths/names in property definitions',
'* platform transforms/transformGroups affecting names, especially when removing specificity',
'* overly inclusive file filters',
].join('\n '));
let warn = `${title}\n ${propertyNamesCollisionWarnings}\n${help}`;
console.log(chalk.keyword(WARNING_COLOR).bold(warn));
}
}


Expand Down
1 change: 1 addition & 0 deletions lib/transform/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed


// The platform can define either a transformGroup or an array
// of transforms. If given a transformGroup that doesn't exist,
Expand Down
1 change: 1 addition & 0 deletions lib/utils/groupMessages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Member

Choose a reason for hiding this comment

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

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.

},

flush: function (messageGroup) {
Expand Down