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

Soft rename addExistingSourceFile to addSourceFileAtPath #733

Closed
dsherret opened this issue Oct 15, 2019 · 2 comments
Closed

Soft rename addExistingSourceFile to addSourceFileAtPath #733

dsherret opened this issue Oct 15, 2019 · 2 comments
Assignees
Milestone

Comments

@dsherret
Copy link
Owner

dsherret commented Oct 15, 2019

I always didn't like this method name. Just thought of this which I think is better.

This should be a soft rename for a long time to allow people ample time to switch.

Would also be nice to have code mods for these changes...

Todo: Consider either addSourceFileAtPath/addSourceFileAtPathOrThrow or addSourceFileAtPath/addSourceFileAtPathIfExists. Probably IfExists is better.

@ChristianIvicevic
Copy link

The first suggestion is more idiomatic with all other methods having orThrow variants. The second suggestion is semantically even different, how would you distinguish the case when you successfully added a source file and the case when you did not because the file did not exist?

Anyways this looks like a simple issue to start contributing with, I'd like to take a look at it once the TODO is done.

@dsherret
Copy link
Owner Author

@ChristianIvicevic yeah, it is more more consistent... I just feel I like the IfExists a bit more in this scenario for the following reasons:

  1. When people use this method they will 99% of the time use the one that always returns a source file (the one that throws). I'd prefer the shorter method name to be the common one and the one to lead people to use.
  2. It's not as clear in this scenario why it throws. Does it throw because something is wrong with parsing? Someone would have to read the JS docs to find out. The IfExists suffix is pretty clear though.

So I know it's inconsistent, but I think the inconsistency is ok here. I've also been playing around with both and I feel like I like the addSourceFileAtPath/addSourceFileAtPathIfExists a bit better.


Sure, that would be great! If you'd like to contribute this is pretty easy. Basically:

  1. Take a look at DirectoryCoordinator.ts and rename those methods.
  2. Follow them out to where they're used in Project and Directory.
  3. For each method in the public api, create a new method with the new name and then on the old method add a @deprecated jsdoc tag saying what the new name is (don't delete it... just point the old method to the new one).

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants