fix: duplicate declarations by naming classes more thoroughly #52
Conversation
transforms/helpers/parse-helper.js
Outdated
| const dirWithoutFile = path.dirname(appFilePath); | ||
| // Only keep no more then the last two directories | ||
| // Ignore any that are plural of the superclass name | ||
| // debugger; |
|
All set. |
|
Ping @pzuraq |
There was a problem hiding this comment.
Can we add a test for something along the lines of app/users/route.js? That will be a common case for pods. I'd also like to test the superclass functionality if that's possible, though I realize that may be trickier. Could require a unit test, and some way to mock the superclass data.
| @@ -473,8 +474,44 @@ function getClassName( | |||
| type = "" | |||
| ) { | |||
| const varDeclaration = getClosetVariableDeclaration(j, eoCallExpression); | |||
| const filePathSplit = filePath.startsWith("app/") | |||
There was a problem hiding this comment.
Might be a good idea to make this configurable, I know some folks put their pods in different directories
There was a problem hiding this comment.
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.
| const fileNameWithoutExt = path.basename(appFilePath, "js"); | ||
| const superClassNameNotInPath = | ||
| superClassName && | ||
| capitalizeFirstLetter(camelCase(fileNameWithoutExt)) !== superClassName; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yeah, maybe a check if the base class is an Ember primitive?
There was a problem hiding this comment.
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.
|
@pzuraq regarding |
| @@ -0,0 +1 @@ | |||
| export default class UsersDefaultImportExportInputRoute extends Route {} | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Is it possible to setup that kind of test, seems output and input need to be used?
There was a problem hiding this comment.
@pzuraq any thoughts on how we'd set this up?
There was a problem hiding this comment.
hmm, this may need a fix upstream, I'll see what I can do
There was a problem hiding this comment.
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
|
This PR works well on files in In |
|
@vlascik think you could take a stab at a PR that fixes it against this PR? |
|
Superceded by #177 |
Resolves #51
This doesn't support MU, only mixed layouts.
Ran it against one app and it transformed 305 files with