Some little nitpicks (typeResolver cleanup in progress) #445
Conversation
2a96d87 to
f5fa9e2
Compare
|
If you're looking at unnecessary typecasting, the type resolver is full of them. if (type.kind === ts.SyntaxKind.Identifier) {
return (type as ts.Identifier).text;
} if (ts.isIdentifier(type)) {
return type.text;
} |
|
I noticed those too, already had the feeling that could be done better. |
| @@ -70,7 +70,7 @@ export class TypeResolver { | |||
| return { | |||
| dataType: 'union', | |||
| types, | |||
| } as Tsoa.UnionType; | |||
| } as Tsoa.UnionType; // XXX why is this necessary? | |||
There was a problem hiding this comment.
Please do not remove as Tsoa.UnionType or any similar stuff
This makes sure all props are set on the returned object.
If I want to return a specific type, I need to make sure all the additional stuff that type requires are added. The cast makes sure this is the case.
There was a problem hiding this comment.
Personally I would prefer typeguards over aliases. Like if we run this through a function that returns the type input is Tsoa.Union and then throw if that result is false, then we get a runtime check and more accurate types.
There was a problem hiding this comment.
This makes sure all props are set on the returned object.
That's (unfortunately) not what this cast does. It's actually a type assertion, meaning that you're basically telling the compiler "Hey, I don't care what you think this is, from now it has this type".
If the types don't match the compiler will sometimes give a warning, but definitely not always (in this instance removing the dataType property didn't trigger any response from the compiler).
In this particular instance the issue was that the return type of the method was set to Tsoa.Type. Tsoa.UnionType is a subclass, but the compiler doesn't consider this (and rightfully so), throwing an error about properties that don't exist in Tsoa.Type. There are two solutions to this:
- Add a type assertion (current situation)
- Change the return type to
Tsoa.Type | Tsoa.UnionType
I think option 2 is better because it more accurately represents the typing and most importantly it actually checks the type.
If the object you're returning doesn't match Tsoa.Type or Tsoa.UnionType it complains. If you're using a type assertion (telling it "this is definitely a UnionType) you won't get any errors.
There was a problem hiding this comment.
A third solution would be something like this:
const someType: Tsoa.SomeType = {
// valid props for this type
};
return someType;This avoids the need for a long union as return type and can be seen as more "readable" due to the explicit mention of what kind of type we're returning. It's however a little clunky in my opinion.
It'd be awesome if Typescript had a weaker kind of type assertion to ask the compiler to verify a type ("Can you check if this matches type X?" instead of "This is type X now"), but I'm not aware of such a feature.
There was a problem hiding this comment.
That's (unfortunately) not what this cast does. It's actually a type assertion, meaning that you're basically telling the compiler "Hey, I don't care what you think this is, from now it has this type". If the types don't match the compiler will sometimes give a warning, but definitely not always (in this instance removing the dataType property didn't trigger any response from the compiler). If the types don't match the compiler will sometimes give a warning, but definitely not always (in this instance removing the dataType property didn't trigger any response from the compiler).
I'd expected it to force me to convert it to unknown first, if it does not, that's unfortunate.
Change the return type to Tsoa.Type | Tsoa.UnionType
Tsoa.Type | Tsoa.UnionType | Tsoa.ArrayType | Tsoa.EnumerateType | Tsoa.IntersectionType | Tsoa.ObjectLiteralType | Tsoa.ReferenceTypeAnd this would change every time someone adds a type. I don't like that very much.
However, Dan has a lot more TS knowledge here so I'd gladly defer to him there.
Maybe it's a good idea to break up the entire method into several smaller ones.
There was a problem hiding this comment.
I'd expected it to force me to convert it to unknown first, if it does not, that's unfortunate.
It surprised me at first too, it's really annoying. In most cases it asks you to convert first, but strangely enough not here.
And this would change every time someone adds a type. I don't like that very much.
I'm not that big of a fan either. However, it allows for stricter type checking which I honestly prefer to having a single long union somewhere that needs to be updated every time a new 'type-type' is added (which shouldn't be that often anyway).
We already have multiple of such unions, and I think that as long as it's properly documented what needs to be updated for new types it shouldn't cause too much hassle. Even without documentation I was able to figure it out pretty quickly when I was implementing tuples.
An even better solution would probably be to refactor to a type system that isn't based on these sorts of 'string literal unions', but that's a discussion for another day.
Maybe it's a good idea to break up the entire method into several smaller ones.
I don't remember exactly what method we're talking about here, but there are definitely a couple of massive methods that could use this.
There was a problem hiding this comment.
Ideally, and if I were to rewrite this code today, I would have Tsoa.Type be a tagged union. There'd be no need for type assertions since you could only return type checked members of the union type.
Type assertions are sometimes necessary but generally to be avoided IMO. It would be quite easy to break this code by extending UnionType in some way since the type assertion will look over the fact that you're missing new or changed properties.
Even though it's uglier, I think a pattern of:
const myType: SomeType;
return myType;
will end up being safer.
There was a problem hiding this comment.
Ideally, and if I were to rewrite this code today, I would have Tsoa.Type be a tagged union. There'd be no need for type assertions since you could only return type checked members of the union type.
Would this be a good change for a possible v3?
Even though it's uglier, I think a pattern of: [...] will end up being safer.
This would be a good point to discuss in the style guide (even though there are objective safety benefits to this approach). I agree that type assertions should definitely be avoided as much as possible because they undermine the typesafety benefits of using Typescript.
In my own projects I always add a comment to each type assertion to explain why this assertion is safe, to make sure others or myself don't get confused or undermine typesafety further.
There was a problem hiding this comment.
Would this be a good change for a possible v3?
Probably. Knowing what I know now, type resolver is just kind of...wrong, but good enough. Redesigning the types around metadata is part of how we can fix it.
I think this particular thing is not so much style, but more of a system design thing. Tsoa.Type is an overly reductive return type because the callers of typeResolver actually care about the subtypes. Having to do the temp variable to avoid the assertion is more of a smell than anything.
There was a problem hiding this comment.
I 100% agree with you. Most of my changes are aimed at making the typeResolver feel less "wrong" (and still good enough), it probably won't be perfect but I hope it'll at least help.
| @@ -86,11 +86,11 @@ export class TypeResolver { | |||
| } | |||
|
|
|||
| if (this.typeNode.kind === ts.SyntaxKind.AnyKeyword) { | |||
| return { dataType: 'any' } as Tsoa.Type; | |||
There was a problem hiding this comment.
While, strictly speaking, this is not necessary, it provides additional info. We are returning the bare base type, not some fancier type, like union, array, etc.
I’m not familiar with ts.isIdentifier so I can’t inform if that’s a better approach. Do you have a resource you would recommend on getting ramped up on the AST parsing functionality of TypeScript’s compiler? My strengths as a maintainer are more in my skills as a pragmatic “product owner” type of person and also my knowledge of OpenAPI/JsonSchema and my love of TypeScript. This is my way of saying that I’d like to become aware of how the TypeScript compiler works, but I don’t know enough to be helpful yet. Please advise. :) |
|
The only official documentation on the compiler API that I know of is this page on the Typescript repository wiki. It isn't that useful though, I got most of my knowledge of the compiler API by trial and error. As for the |
For Visualization: For the typescript compiler API: |
|
Little status update on the cleanup: I'm almost done, just 6/24 methods left of which 2 I've already done once but in a way that caused tests to fail. There's however one set of methods that I can't figure out: |
| return JSON.stringify(context); | ||
| }); | ||
| const additionalPropsHelper = (additionalProperties: TsoaRoute.ModelSchema['additionalProperties']) => { | ||
| handlebars.registerHelper('json', (context: any) => JSON.stringify(context)); |
There was a problem hiding this comment.
Please remove these style changes. There’s nothing wrong with this code functionally. Also please consider that for the entire PR, since I won’t approve a PR that changes code that didn’t need to be changed. And to clarify, code only needs to change if it either fixes a known issue (that has an issue number in tsoa’s github) or if it enhances tsoa (and therefore has a tsoa issue number with the discussed enhancement).
There was a problem hiding this comment.
(Whoops I see I'm responding to your comments in reverse order, you probably want the read my comment below (the long one) before this.)
I think I already touched upon most of what you're saying here in my long comment below, but just for clarification's sake:
There’s nothing wrong with this code functionally
No there's nothing wrong functionally, but there's a lot of room for easy improvement in readability, accessibility and maintainability.
Also please consider that for the entire PR,
As I said in my second comment (from below, I really should've answered these in the correct order...) I'm trying to keep (habitual) stylistic changes to a minimum, I'll remove any that snuck in after I'm done.
since I won’t approve a PR that changes code that didn’t need to be changed.
I'd argue some of these changes are definitely needed (or at least welcome improvements). I don't see how rejecting a change just because it "changes what's already there" helps with improving software. Endless cycles of refactoring are of course undesirable, but a rigorous review & cleanup can be very healthy for a project (at least in my experience).
And to clarify, code only needs to change if it either fixes a known issue (that has an issue number in tsoa’s github) or if it enhances tsoa (and therefore has a tsoa issue number with the discussed enhancement).
I already touched upon this in my next comment (sorry again).
There was a problem hiding this comment.
Please understand that I personally wrote the code that you changed. I wrote it that way because it made it shorter horizontally. That’s because I subjectively felt that that they way I wrote the code was better. You have now changed it to what you think is subjectively better. If I was getting paid as part of my day job then I would happily debate with you the merits of breaking code into discrete parts.
Every change that is submitted requires my review. So if a change is submitted for subjective/stylistic reasons, that takes up valuable time of mine that I could be spending reviewing or improving one of the many open issues that tsoa has.
I’m summary, do not change code that does not need to be changed. This was recently reviewed by Endor and WoH and they didn’t have problems with it. We will only accept changes that improve the code objectively. And the only objective improvements that can be made are bug fixes or feature enhancements.
There was a problem hiding this comment.
I wrote it that way because it made it shorter horizontally. That’s because I subjectively felt that that they way I wrote the code was better. You have now changed it to what you think is subjectively better.
This is of course true, but it's not like I'm changing it purely because I think it "looks better", none of these changes are. In this specific case I changed it because it avoids an additional (completely useless) variable. This results in more concise code that's easier and faster to understand, because you skip the "where does this variable come from" and "what is this variable used for" steps. It's also more consistent with the rest of the codebase, as well as most other code I've encountered. Honestly, I prefer that to saving a couple characters horizontally.
But there's really no use to arguing about this now, like luke said we need a style guide for things like this so we can make "informed" or "standardized" (not sure what the appropriate word is here) but most importantly consistent choices.
Every change that is submitted requires my review. So if a change is submitted for subjective/stylistic reasons, that takes up valuable time of mine that I could be spending reviewing or improving one of the many open issues that tsoa has.
While true, I don't think this is a valid reason for rejecting an honest try at improving the codebase. Having a style guide will of course speed up the reviewing process since you can make an objective decision based on this guide.
Also there's another benefit to making the code more contributor friendly: a bigger pool of people that can/want to review PR's. (I'm not saying this PR will immediately solve this issue, I just hope that it opens up the possibility of long term improvements.)
I’m summary, do not change code that does not need to be changed. [...] We will only accept changes that improve the code objectively. And the only objective improvements that can be made are bug fixes or feature enhancements.
Again, most of my changes are in fact objective improvements (typesafety). Changes like the one you selected here might not be entirely objective but there's definitely an (in my opinion) valid reasoning behind it. As luke said, we need a style guide.
I also want to add a short note that I think that maintainability, contributor-friendliness and readability are in fact objective improvements, but the line you selected here isn't really a good example of that.
This was recently reviewed by Endor and WoH and they didn’t have problems with it.
I don't see how "it was approved once" clears it from any kind of improvement. Not all code that's "approved" is perfect (for example due to sloppy reviewing or just not having an optimal solution), I'll take my own shameful #448 as example.
Don't get me wrong on this, I'm not saying that this code is bad or that Endor/WoH haven't reviewed your code properly, I'm just trying to say that I think we should keep an "open mind" about incoming improvements.
There was a problem hiding this comment.
In this specific case I changed it because it avoids an additional (completely useless) variable.
For you that variable is useless. For me it added readability.
There are some people that would prefer to solve an algebra problem as follows:
x = 2 * 3 + 4 - 6 * 7
x = -33
But I always prefer the following approach. And my algebra teacher recommended this approach as well always saying “show your work”:
x = 2 * 3 + 4 - 6 * 7
x = (2 * 3) + 4 - (6 * 7)
x = 5 + 4 - 42
x = -33
So again, this is a subjective point. Not one that should cause a change without a bug or an enhancement to give a reason to change it.
Contributions to software should be like surgery. If I have to do surgery, I will fix things related to the problem and that the patient has pre-approved. I don’t just fix the lungs while I’m doing open heart surgery. In the same manner, software contributions should be small, atomic, and should be specific to the issue at hand. So since this line has nothing to do with improving the type safety of the type resolver, then it should not have been changed.
If however there is an open issue related to the route file generation, then I would be more than happy to entertain refactoring on this file. I will indeed have an open mind when surgery on the routeGenerator is necessary.
There was a problem hiding this comment.
For yo that variable is useless. For me it added readability.
I may have worded my point of view a little too strong, there's definitely room for debate here. As for the math analogy: I agree with you, but more steps doesn't equal more readability. There's a point where code becomes too convoluted for it's own good. Where exactly that point is should be added in the style guide.
In my opinion this particular piece of code crossed that line, for reasons I'm not going to repeat again. Increased readability/maintainability/accessibility is an enhancement in my opinion, which is why I chose to change it at that particular point in time (which for the record was months ago when I first saw the codebase).
Contributions to software should be like surgery.
I strongly disagree with this analogy. I think it's easiest to explain my viewpoint with another analogy:
I imagine software more like a computer. If it's not broken you don't want to be constantly changing it, that's just unnecessary hassle. Switching out the parts, verifying everything is still working, installing new drivers, the whole bunch.
However, once in a while you might want to do maintenance to improve your experience. Maybe add a little extra storage, a faster CPU or more RAM. While you're doing these upgrades you might find the cable management to be a little "lacking" so to speak, so you decide to clean that up as well.
Even though redoing the cable management might not be much of a tangible enhancement or a fix for a concrete problem you're having right now, it will still definitely benefit you in the long run. Because the next time you want to add some more RAM or switch out a broken hard drive you want to be able to focus on that, instead of having to navigate through or possibly even redo the messy cabling.
In the same manner, software contributions should be small, atomic, and should be specific to the issue at hand. So since this line has nothing to do with improving the type safety of the type resolver, then it should not have been changed.
I agree with this bit in the sense that I should've split this out more from the start. I messed up in that area, and I'm sorry about that. However, that doesn't mean the changes I'm proposing should be outright rejected.
All things considered, I don't think there's many use in continuing this debate any further. Like luke said, we should get a style guide and have an open discussion about the contents of it.
| @@ -143,19 +141,20 @@ export class RouteGenerator { | |||
| properties[property.name] = this.buildPropertySchema(property); | |||
| }); | |||
| } | |||
| const modelSchema = { | |||
|
|
|||
| models[name] = { | |||
There was a problem hiding this comment.
This is another stylistic change. Code should only be changed if it either fixes a known issue (that has an issue number in tsoa’s github) or if it enhances tsoa (and therefore has a tsoa issue number with the discussed enhancement).
There was a problem hiding this comment.
(Whoops I see I'm responding to your comments in reverse order, you probably want the read my comment below (the long one) before this.)
This is another stylistic change.
I definitely would not call this a stylistic choice. I removed an additional (completely unnecessary) variable to make the code more concise and readable (at least in my opinion). You could argue that it doesn't improve these things, but I definitely wouldn't call it a stylistic choice.
Code should only be changed if it either fixes a known issue (that has an issue number in tsoa’s github) or if it enhances tsoa (and therefore has a tsoa issue number with the discussed enhancement).
Does improving the code readability/accessibility fall under 'enhancement'? Then I definitely agree with you.
I could make an issue called "The codebase is confusing" but I don't really see the point in doing that if I already have some small changes laying around that improves it. If I were to do a bigger cleanup I would have created an issue. In hindsight I maybe should have done that for my typeResolver cleanup, but my original idea would be to only fix those missing type guards. Unfortunately I discovered a lot of weird typing (mistakes) and I just sortof, well, fell into the rabbit hole.
I think you're massively overestimating the size of this cleanup, 95% is just typing fixes.
| @@ -108,7 +108,7 @@ export function getParameterValidators(parameter: ts.ParameterDeclaration, param | |||
| ); | |||
| } | |||
|
|
|||
| export function getPropertyValidators(property: ts.PropertyDeclaration): Tsoa.Validators | undefined { | |||
There was a problem hiding this comment.
Please explain why undefined is now an acceptable response?
@HoldYourWaffle, as a heads up, I’m unlikely to approve a PR that does a refactor of the entire code base without any clarity on what bug it fixes or what feature it adds to tsoa. I’ll give you time to respond, but it’s likely that I will close this PR. I would however approved a smaller PR if it:
- targets one small feature of tsoa
- increases the automated code coverage for that feature
Ultimately I like to see software grow and iterate in small cycles not in “Big Bang” refactors. I’m saying this because you said that you have a large amount of changes coming.
By the way, tsoa will soon have PR and issue templates that clarify these contribution guidelines.
There was a problem hiding this comment.
Please explain why undefined is now an acceptable response?
I'm not sure what you mean by this. If I remember correctly the undefined return type was "unused" because it would always return at least {}, it's just a typing fix.
I’m unlikely to approve a PR that does a refactor of the entire code base
Well it's good I'm not doing that then 😉 I'm just cleaning up the type resolver for typesafety enhancements/fixes and especially readability. I also added some notes on uncovered edge-cases I think I might've found or pieces of code that were confusing to me (so they can be better documented in the future).
without any clarity on what bug it fixes or what feature it adds to tsoa
As I said before, it increases readability (a lot in my opinion) and makes the code more typesafe. Sorry for not mentioning those reasons earlier, I thought this (the initial motivation) and this comment had made that clear already.
I’ll give you time to respond
This isn't meant in any negative sense, just out of honest curiosity: how long would you have waited for my response? You seem to be pretty quick on these sort of things.
Ultimately I like to see software grow and iterate in small cycles not in “Big Bang” refactors. I’m saying this because you said that you have a large amount of changes coming.
This is definitely not a "big bang refactor", I'd hardly even call it a refactor. I'm also not sure where you got the idea that "large changes were coming", did I choose my wording poorly somewhere? If so please tell me, I'm just trying to learn.
I'm only doing two things right now:
- Fixing some typing mistakes I noticed (I don't see a reason why this would ever be a bad thing)
- Making the code more readable by adapting more modern approaches and conciser code.
Especially the second point is important for ensuring this project stays maintainable and most importantly approachable for new contributors.
The current code is not very newcomer-friendly (at least in my experience) because it contains all kinds of unnecessary "bloat constructions" such as:
- [extra variable + type assertions] instead of type guards
- One-use variables that could perfectly be inlined (more code to read and interpret, stuff like "what is this variable for?")
- Unnecessary
as anyor even blatantly wrong assertions that effectively ruin typesafety
There are more of these but these three are the most common (and in my opinion the most annoying) ones. They're not just stylistic choices, they make the code objectively less readable (although you could argue about that for the second type).
I won't pretend I didn't make some stylistic changes, but that's mostly out of habit. I'll do a full passover once I'm done to remove any of these pure stylistic changes.
The changes you're seeing right now are just some little mistakes/improvements I found/made while exploring the codebase for the very first time. They are meant to make the code easier to understand (at least for an outsider), most often by reducing/removing these unnecessary bloat constructions.
As long as these changes don't:
- Cause bugs (which we have tests for [I promise I'll make sure they run this time])
- Increase confusion (which can vary from person to person, feedback is always welcome)
- Decrease test coverage
- Undermine typesafety (which is the opposite of what I'm doing)
- Reduce maintainability (I'm trying to increase it)
- Make the code less accessible for newcomers (Also doing the opposite of this)
- Reduce flexibility for no reason
- Increase complexity
- Decrease documentation
- Significantly change the layout/architecture of the project (which is what I would call a "(big bang) refactor")
I don't see why that's a bad thing.
There was a problem hiding this comment.
My two cents: if you say the codebase is confusing, then it is confusing. A lot of this code is cobbled together from (sometimes not very well thought out) bug fixes, so I trust that this all comes from a good place.
I think as a point of discipline, the type fixes should be their own PR as they represent no change in runtime behavior and should be uncontroversial if they are, indeed, corrections.
Other stuff like code style is fine but we should probably open up a broader discussion about how we want to approach style conventions. I was pleased to see that we added prettier. I think we should probably just pick someone else's existing style guide and apply it universally; to the extent possible, I don't want us arguing over things that don't result in some benefit for the end-user.
As it stands, I don't think we have a great contributor guide, so that's something we can work together to produce. For a long time, it wasn't necessary with me as benevolent dictator, but now that we've got several folks getting really active, we need to establish some conventions.
There was a problem hiding this comment.
My two cents: if you say the codebase is confusing, then it is confusing. A lot of this code is cobbled together from (sometimes not very well thought out) bug fixes, so I trust that this all comes from a good place.
Thank you for joining in on this. The code reads to me exactly as you described it, hacked together with sometimes not-100%-perfect solutions. The idea is there, and it's not extremely terrible (after all I did get through it) but there's definitely some room for improvement.
I'm honestly just trying to help improve this project, nothing more nothing less. I think it's an amazing concept and I hope that by making the code easier to understand it can be more accessible for future contributors (myself included).
I think as a point of discipline, the type fixes should be their own PR as they represent no change in runtime behavior and should be uncontroversial if they are, indeed, corrections.
I thought about doing that too. If it doesn't create too much of a conflict hell I'll try to split out these parts of the PR.
Other stuff like code style is fine but we should probably open up a broader discussion about how we want to approach style conventions.
Agreed. I think most of my changes of which you could possibly say that they're "stylistic" rest on the same set of "style choices". If we decide on which style we want, we can reject/approve a whole set of these changes at once. Again if it doesn't create too much of a commit hell I might be able to split-out all these "style choices" (if you really want to call it that way) into separate commits.
I don't want us arguing over things that don't result in some benefit for the end-user.
I definitely agree, as long as there's still room for maintainability improvements such as these (referring to the typing changes/improvements).
There was a problem hiding this comment.
I thought about doing that too. If it doesn't create too much of a conflict hell I'll try to split out these parts of the PR.
It shouldn't be too bad. When in doubt, split it out ✂️
I definitely agree, as long as there's still room for maintainability improvements such as these (referring to the typing changes/improvements).
Yeah, my point here is that we should just have a style guide with these things documented so at code review time there isn't even a discussion to be had. I opened #449 to kick off a high level discussion of how we might iterate on this idea.
There was a problem hiding this comment.
I agree. I'll add some of my opinions to your issue shortly.
| localReferenceTypeCache[refNameWithGenerics] = referenceType; | ||
|
|
||
| if (example) { |
There was a problem hiding this comment.
Why did you remove this section?
| additionalProperties, | ||
| dataType: 'refObject', | ||
| description: this.getNodeDescription(modelType), | ||
| example, |
There was a problem hiding this comment.
@dgreene1 I put it here. Oddly enough I changed it here instead of just adding a note asking what's the difference between checking for undefined (or technically falsy values) before assigning. Must've been late when I did this.
|
Another status update: I think I've managed to split out my cleanup quite reasonably. Not everything is done perfectly (not all intermediate commits pass tests) but it should be sufficient for a review. I did manage to get the formatting correct on every intermediate commit, which I'm quite proud of (it caused quite a few conflicts). Unfortunately it's not reasonably possible to create separate PR's for purely uncontroversial changes and everything else ranging from "I think almost everyone agrees with this" to "I guess this could be seen as a stylistic change" because commits depend on each other and I can't get git to split its diffs small enough. I really hope this is not too big of an issue. I'd recommend reviewing the changes per-commit anyway since it's mostly like-minded changes grouped together. |
|
@HoldYourWaffle, Luke meant that you should split this into multiple PRs for each related issue. I don’t see any new issues or PRs, so I’m confused. |
|
I literally just explained that it's not reasonably possible to split out these changes into different PR's because commits depend on each other. Fixing this is near-impossible and would require a lot of manual merging and conflict resolution, something I'm not keen on doing if it's not absolutely necessary. I don't think it's necessary in this situation, because instead of having separate PR's for each issue, there's now a separate (deletable) commit for each type of change.
I haven't pushed it yet 😉 Still need to fix up some minor things. Edit: actually, thinking about this, it does make sense to have an issue for each commit to organize reasoning and discussion. I'll get to it once I push my code. |
I’ve been very clear that every PR must have an issue. Luke chimes in as well.
Thank you. This is standard practice for most of the open source libraries I contribute to and/or maintain. |
|
I’ll be mentioning this in our upcoming contribution guideline, but it’s also standard practice to not allow PRs that don’t add functionality or fix issues. I’m sharing the following because I want you to feel appreciated. I also want you to recognize that the rationale that we are using is standard and respected. For more information, see the ElasticSearch contributor guidelines:
https://github.com/elastic/elasticsearch/blob/master/CONTRIBUTING.md |
|
@HoldYourWaffle now that I’ve provided rationale and examples of why a PR that “refactors for refactors” sake is problematic, I am going to close this PR. However, I believe that you brought up some excellent points about the type resolver. I have made a feature request #452 specifically with you in mind. If you would like to add the unknown type feature, it would be a perfect opportunity to refactor the type resolver to use a tagged union as Luke suggested (#445 (comment)). It can be done in a backwards compatible way that does it require a major version. So in the meantime, I would enjoy your input in the unknown type issue thread.. Once we agree on the desired outcome, we will set the “help wanted” tag on the issue so that it’s clear to everyone that a PR is welcome. This follows the TypeScript contributor guidelines that Luke provides:
Thank you for using tsoa and for being a part of the community. I believe that following these standard practices will save everyone time (including yours). I thought that these guidelines were universal, but Luke is right that we should canonize them. So please accept my apologies that we did not have clear contributor guidelines at the time you submitted this PR. |
|
Edit: seeing the size of this thing, I probably should've put it in #449 😅
Oh I know it's standard for bigger projects/PR's, I just didn't (initially) think these changes were significant enough to have their own issue (like #450 and #248 for example). Keep in mind that this was originally meant as "I found some fixes for mistakes and weird things I had laying around, maybe they're useful?" (I think they definitely are). Unfortunately some stylistic changes crept in (I should've watched out for that more) and I maybe shouldn't have jumped the gun so quickly on cleaning up the type resolver. I was originally just going to change the I do however think that (most of) my changes are useful, which is why I still want to PR them now that I've split them up properly.
Just because something is "standard practice" doesn't mean it's a good idea per se (although big chance it is otherwise it probably wouldn't have become standard). The guide you linked for Elasticsearch looks very good, but it was made specifically for Elasticsearch (with some basic standard practices included of course). Elasticsearch is a very different project from TSOA, with almost 26x the amount of contributors (including non-active ones), 52x the amount of open issues and 19x times the amount of open pull requests. Code quality is another important factor. If the code is already of high quality (which I imagine ElasticSearch would be) there's not much reason to change it. Unfortunately this simply isn't the case for TSOA (at least in most areas I checked) based on typesafety mistakes and unnecessary casts alone. If you reject refactors "because you don't want refactors", you're effectively rejecting any kind of improvement to the (very improvable) quality of the code base.
There's a difference between "refactors for the sake of refactoring" and "small restructures to make the code a lot more readable/accessible". As I explained in my previous point, I think it's important for smaller projects like this one that we keep an open mind about the latter (especially if there's a lot to be won). Now that I've broken up the changes it shouldn't be a lot of effort to approve/reject individual types of changes. Over half of these changes are very simple (and small) adjustments I made to make the code more readable/accessible (at least in my opinion). They can easily be approved or rejected by either a "yes" or "no" (although I prefer some reasoning of course).
Just as with Elasticsearch, Typescript is a much, much larger project than TSOA. Big projects need this kind of bureaucracy to keep things sane, I simply don't think we're at that level with TSOA (yet). A "bugfixes only" or "marked as 'help wanted' only" rule makes sense for them because they have a large team of their own working on it fulltime and the codebase is already of very high quality. Unfortunately we have neither of those things, so I don't see the specific need for such hard rules.
I do consider them universal, but for big projects (like Elasticsearch and Typescript). For small changes (in small projects) like the ones this PR was originally meant for it just seems like a lot of unnecessary bureaucracy to me (up to a certain point of course) that keep the code from organically improving on itself. Of course if you really want to implement such rules there's nothing I can do about it, after all luke has the final say as "benevolent dictator". However, I believe that it's very important that everyone is able to voice their opinion in order to get the best outcome for the project in the long run. After all, the only thing I want and the reason why I started this "discussion" (although I'd definitely call it a healthy debate) is to get a fair review of my changes, because it felt like you were rejecting them "because they were refactors" (the odd stylistic change that crept in excluded of course). I think these changes are still valuable to the project (especially [parts of] my typeresolver cleanup), and I'd hate for them to be rejected just because I proposed them I a not-very-smart-or-handy-way™. |
These are just some very small nitpicks and notes I put in when I first explored the codebase 2 months ago, as well as the linter doing something when I ran yarn for the first time.
This isn't really PR-worthy, but I'm creating it anyway to avoid conflicts with my other (in-progress) branches.
I also have one tiny question about the code:
tsoa/src/routeGeneration/routeGenerator.ts
Lines 172 to 187 in 0067d08
Why is the length of the keyset checked here (line 182)? In what situation would the
source.validatorsobject have 0 keys but a value that's unwanted forparameterSchema.validators?I also noticed that there are a lot of (most likely unnecessary) type assertions scattered around the codebase. It might be good to do a full check on these and remove any of these unnecessary liabilities (I have already removed one in this PR).