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.

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

variable declarations for modules and namespaces #32

Merged
merged 2 commits into from
Dec 7, 2017

Conversation

wehrstedt
Copy link
Contributor

This PR adds support for variable declarations inside namespaces and modules

@wehrstedt
Copy link
Contributor Author

I hope it's okay that I added so much dependencies but they were necessary for the tests 🙈

@RyanCavanaugh
Copy link
Owner

Code looks good.

Not really a fan of the tests - asserting the internal structure of the DOM isn't really testing, and a string compare of the output is going to be very sensitive to formatting changes. A baselining system would be more appropriate for this library. Have the unit tests write out their output to a file that's checked in, and humans can verify the output / see the diffs in source control if they change later.

@wehrstedt
Copy link
Contributor Author

Okay, with the first point you are right. That's what the types and interfaces in typescript are made for (As an excuse, I'm a javascript developer, I'm used to check everything I work with 😄 )

The second point:
In my opinion, you move the "testing" to the source control instead of using automated systems like the mocha testframework which can tell you that some specific components were changed. Maybe the tests are not detailed enough.

Your approach is quite similar to mine, with the difference that you can see the changes easier in form of a written file, but a source control is not made for detecting changes of build files (just my opinion).

I can change or remove the tests if you like?

@RyanCavanaugh
Copy link
Owner

If the output of the generator were unambiguously the only possible correct output from the input (i.e. 2+2 is unambiguously 4), this approach would be OK. But there are many equally correct ways to write the same .d.ts file from the same DOM input.

What happens if we add hundreds of tests like this, then decide that the default output formatting should place the brace on the next line, or that large type aliases of unions should be split into multiple lines? Now we have to hand-edit hundreds of string literals in a .ts file; that's a nightmare.

@wehrstedt
Copy link
Contributor Author

I understand your point. And your totally right.
But that's the typical never ending discussion where the arguments of both sides are well comprehensible.

That's why I said, maybe the tests are not specific enough. You could also remove every whitespace and linebreak of the output to compare only the relevant data. Or implement the test to handle multiple representations.

On the other side you can say: Why should I change the indention from tab's to whitespace or vice versa? Or why should I decide to change the brace position? That are fundamental decisions I made at the beginning.
Personaly, I want to have always the same output with the same formatting. For example its inconsistent for me if you place the brace on the same line for class definitions but on the next line for namespaces.
You have to decide if it's important for you that your output has always the same representation or if its not important if the brace position differs for different types.

For me, it's important, but maybe im just a perfectionist.
But as I said, you will always find pro's and contras for both sides which are more or less comprehensible

@wehrstedt
Copy link
Contributor Author

How should we continue? I need these changes for my project..

@RyanCavanaugh
Copy link
Owner

I consider the tests as written to have negative value, but would take the non-test changes as-is. The test code can just run the constructed DOM objects through the generator to validate that it doesn't throw and we can work on a validation strategy later. Thoughts?

@wehrstedt
Copy link
Contributor Author

Ok. I commented out the assertions and removed the first "test". Maybe they are usefull later.
I just added the tests because I had to write a script to test my changes anyway 😄

@RyanCavanaugh
Copy link
Owner

Thanks!

@RyanCavanaugh RyanCavanaugh merged commit 7cc2205 into RyanCavanaugh:master Dec 7, 2017
@RyanCavanaugh
Copy link
Owner

Published 0.1.22

This was referenced Jan 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants