Re: [FalconJx] Refactor Notes of the last couple commits
Your right about the abstract, that is not something we want to force since its on demand invoking. They should be protected and pass the collection though. As I said, the 'as' and 'js' packages within compiler was my mistake. I agree that once things get more stable we should talk about one final refactor that moves everything to their most logical placements within the compiler hierarchy. I work with people to the point of idealism Erik, I will never understand the way somethings get developed in this world, such waste of resources. Communication saves resources, especially that most valuable resource, time. Mike Quoting Erik de Bruin : Mike, - 'common' Although I agree with you making things common, I don't agree with having sub packages, it seems redundant and confusing, if a sub package warrants in common, it should have a real package in compiler. Might be non-native speaker choosing names... The code I put in 'common' is code that 'as' and 'mxml' have in common. In that sense, for symmetry's sake, I thought it should also have the same sub package structure as both 'as' and 'mxml'. Maybe 'shared' or 'general' might have been better names for the package? - On the note of this change, if we could have talked about this first you would have heard me first say that maybe we should move 'compiler.as' and 'compiler.js' into a 'compiler.codegen' ... The same change could be applied to 'driver'. This was a mistake on my part when I was originally laying out the first impl of the packages. I see two "mistakes" (your original layout and me blindly copying/extending it) combined creating one humongous refactor when things quiet down a bit ;-) - Tests, I don't understand why addLibraries() and such in ITestBase are public API. They will never be called outside of the test. In java you would make them abstract and the TestBase class abstract and that creates the subclass contract implicitly. At least one of them has an implementation in TestBase, and the others may well have one in the future... I'm not that familiar with Java - and AS has no 'abstract' - but wouldn't having an implementation prohibit the declaration from being abstract? - Also, passing the List as a parameter of those 3 methods encapsulates the actual field, then if you are just overridding them in a sub class, you are not trying to figure out what field goes where, its just a template method that you add entries to the list passed. That was your original implementation, but again my lack of familiarity with Java is probably causing me some problems here. I figured that calling 'super' with the list as argument might cause problems. In hindsight, it might have been a different aspect of my implementation that was causing the problem, prompting the change and loosing the advantage of the original approach (would this still work when using ITestbase?) I hope you can think about these issues, maybe we can change them down the road. Right now there are no reverting necessary. Excellent! EdB -- Ix Multimedia Software Jan Luykenstraat 27 3521 VB Utrecht T. 06-51952295 I. www.ixsoftware.nl
Re: [FalconJx] Refactor Notes of the last couple commits
Mike, > - 'common' Although I agree with you making things common, I don't agree > with having sub packages, it seems redundant and confusing, if a sub package > warrants in common, it should have a real package in compiler. Might be non-native speaker choosing names... The code I put in 'common' is code that 'as' and 'mxml' have in common. In that sense, for symmetry's sake, I thought it should also have the same sub package structure as both 'as' and 'mxml'. Maybe 'shared' or 'general' might have been better names for the package? > - On the note of this change, if we could have talked about this first you > would have heard me first say that maybe we should move 'compiler.as' and > 'compiler.js' into a 'compiler.codegen' >... > The same change could be applied to 'driver'. This was a mistake on my part > when I was originally laying out the first impl of the packages. I see two "mistakes" (your original layout and me blindly copying/extending it) combined creating one humongous refactor when things quiet down a bit ;-) > - Tests, I don't understand why addLibraries() and such in ITestBase are > public API. They will never be called outside of the test. In java you would > make them abstract and the TestBase class abstract and that creates the > subclass contract implicitly. At least one of them has an implementation in TestBase, and the others may well have one in the future... I'm not that familiar with Java - and AS has no 'abstract' - but wouldn't having an implementation prohibit the declaration from being abstract? > - Also, passing the List as a parameter of those 3 methods encapsulates the > actual field, then if you are just overridding them in a sub class, you are > not trying to figure out what field goes where, its just a template method > that you add entries to the list passed. That was your original implementation, but again my lack of familiarity with Java is probably causing me some problems here. I figured that calling 'super' with the list as argument might cause problems. In hindsight, it might have been a different aspect of my implementation that was causing the problem, prompting the change and loosing the advantage of the original approach (would this still work when using ITestbase?) > I hope you can think about these issues, maybe we can change them down the > road. Right now there are no reverting necessary. Excellent! EdB -- Ix Multimedia Software Jan Luykenstraat 27 3521 VB Utrecht T. 06-51952295 I. www.ixsoftware.nl
[FalconJx] Refactor Notes of the last couple commits
Erik, Now that I have got my project's tests passing again, I thought I would voice some of my opinions in a more professional manner after the storm. - 'common' Although I agree with you making things common, I don't agree with having sub packages, it seems redundant and confusing, if a sub package warrants in common, it should have a real package in compiler. - On the note of this change, if we could have talked about this first you would have heard me first say that maybe we should move 'compiler.as' and 'compiler.js' into a 'compiler.codegen' Existing org.apache.flex.compiler.as.codegen org.apache.flex.compiler.js.codegen To; org.apache.flex.compiler.codegen.as org.apache.flex.compiler.codegen.js Which then would have allowed; org.apache.flex.compiler.codegen.IEmitter org.apache.flex.compiler.codegen.IDocEmitter org.apache.flex.compiler.codegen.IEmitterTokens The toplevel codegen becomes the 'common' container. The same change could be applied to 'driver'. This was a mistake on my part when I was originally laying out the first impl of the packages. - The above I will argue for the 'driver' package as well. - Tests, I don't understand why addLibraries() and such in ITestBase are public API. They will never be called outside of the test. In java you would make them abstract and the TestBase class abstract and that creates the subclass contract implicitly. - Also, passing the List as a parameter of those 3 methods encapsulates the actual field, then if you are just overridding them in a sub class, you are not trying to figure out what field goes where, its just a template method that you add entries to the list passed. To me this is enough merrit for at least a discussion about a veto. I hope you can think about these issues, maybe we can change them down the road. Right now there are no reverting necessary. Mike -- Michael Schmalle - Teoti Graphix, LLC http://www.teotigraphix.com http://blog.teotigraphix.com