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

Jump to bottom

fix: duplicate declarations by naming classes more thoroughly #52

Closed
wants to merge 2 commits into from
Closed

fix: duplicate declarations by naming classes more thoroughly #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7bf961f
fix: duplicate declarations by naming classes more thoroughly
knownasilya Jan 17, 2019
ecead9c
chore: remove debugger
knownasilya Jan 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions transforms/ember-object/__testfixtures__/app/routes/default-import.export.input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default Route.extend({});
1 change: 1 addition & 0 deletions transforms/ember-object/__testfixtures__/app/routes/default-import.export.output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default class DefaultImportExportInputRoute extends Route {}
1 change: 1 addition & 0 deletions ...sforms/ember-object/__testfixtures__/app/user/edit/profile/default-import.export.input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default Route.extend({});
1 change: 1 addition & 0 deletions ...forms/ember-object/__testfixtures__/app/user/edit/profile/default-import.export.output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default class EditProfileDefaultImportExportInputRoute extends Route {}
1 change: 1 addition & 0 deletions transforms/ember-object/__testfixtures__/app/users/default-import.export.input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default Route.extend({});
1 change: 1 addition & 0 deletions transforms/ember-object/__testfixtures__/app/users/default-import.export.output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default class UsersDefaultImportExportInputRoute extends Route {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test does test whether or not the folder and the file name will be prepended to the class, but I think we should explicitly test the case where the file name is the name of the class/type (e.g. route.js). This is definitely going to be the most common case with pods, and I'd like to make sure that it's under test.

Copy link
Author

Choose a reason for hiding this comment

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

Is it possible to setup that kind of test, seems output and input need to be used?

Copy link
Author

Choose a reason for hiding this comment

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

@pzuraq any thoughts on how we'd set this up?

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, this may need a fix upstream, I'll see what I can do

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just recording here real quick because I'm not sure when @rwjblue or I will have time do it, but we chatted and figured the best solution here would be to rename the input files so that they drop the .input part before running tests in codemod-cli

1 change: 1 addition & 0 deletions transforms/ember-object/__testfixtures__/app/users/index/default-import.export.input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default Route.extend({});
1 change: 1 addition & 0 deletions transforms/ember-object/__testfixtures__/app/users/index/default-import.export.output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default class UsersIndexDefaultImportExportInputRoute extends Route {}
40 changes: 39 additions & 1 deletion transforms/helpers/parse-helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const path = require("path");
const camelCase = require("camelcase");
const pluralize = require("pluralize");
const {
capitalizeFirstLetter,
DECORATOR_PATHS,
Expand Down Expand Up @@ -473,8 +474,44 @@ function getClassName(
type = ""
) {
const varDeclaration = getClosetVariableDeclaration(j, eoCallExpression);
const filePathSplit = filePath.startsWith("app/")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be a good idea to make this configurable, I know some folks put their pods in different directories

Copy link
Author

Choose a reason for hiding this comment

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

This should be the case for everyone, even if their pods are in a different directory. The cases this would need to change for is engines, dummy app, and MU.

? filePath.split("app/")
: filePath.split("/app/");
let normalizedName;

// Has app in path
if (filePathSplit.length === 2) {
const appFilePath = filePathSplit[1];
const dirWithoutFile = path.dirname(appFilePath);
// Only keep no more then the last two directories
// Ignore any that are plural of the superclass name
const lastTwoDirsOrLess = dirWithoutFile
.split("/")
.slice(-2)
.filter(
dir =>
capitalizeFirstLetter(camelCase(dir)) !== pluralize(superClassName)
);

const fileNameWithoutExt = path.basename(appFilePath, "js");
const superClassNameNotInPath =
superClassName &&
capitalizeFirstLetter(camelCase(fileNameWithoutExt)) !== superClassName;
Copy link
Collaborator

Choose a reason for hiding this comment

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

My understanding here is that superClassName may not always be a base class like Route or Controller. It could be a different base route class defined within the app. Does this make sense still in that case?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, maybe a check if the base class is an Ember primitive?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to figure out what the super class is, and what the "kind" is (e.g. Route, Controller, Component, etc). This can be done during our analysis with Dyfactor pretty easily, we may just need to record both and pass them forward.


lastTwoDirsOrLess.push(fileNameWithoutExt);

// file name isn't already the type so add thetype to the name it
if (superClassNameNotInPath) {
lastTwoDirsOrLess.push(superClassName);
}

normalizedName = lastTwoDirsOrLess.join("-");
} else {
normalizedName = path.basename(filePath, "js");
}

const className =
getVariableName(varDeclaration) || camelCase(path.basename(filePath, "js"));
getVariableName(varDeclaration) || camelCase(normalizedName);
let capitalizedClassName = `${capitalizeFirstLetter(
className
)}${capitalizeFirstLetter(type)}`;
Expand Down Expand Up @@ -569,6 +606,7 @@ function replaceEmberObjectExpressions(j, root, filePath, options = {}) {
}

const superClassName = get(eoCallExpression, "value.callee.object.name");

const es6ClassDeclaration = createClass(
j,
getClassName(
Expand Down