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

Changes to use promises instead of callbacks #6

Closed
wants to merge 4 commits into from
Closed

Conversation

pranavkm
Copy link
Contributor

Felt that it was slightly cleaner and a bit more readable. I couldn't get the tests to run though, so this is still work in progress.

@@ -9,3 +9,5 @@ _ReSharper.*/
node_packages/*
node_modules/*
lib/*.js
bin/*.js
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove this added line.
Since we use the precompiled approach (typescript is compiled before being packaged) we check in the resulted (after running build) kuduSync.js file.

@davidebbo
Copy link
Member

I don't have the background on this. It looks like a huge change, and on the surface, it being "slightly cleaner and a bit more readable" seems like a weak reason. Let's discuss the pros and cons.

@@ -1,4 +1,4 @@
///<reference path='attempt.ts'/>
///<reference path='Utils.ts'/>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lowercase u utils.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly there's a mismatch in the actual file name casing (e.g. FileInfo.ts and the reference reference path='fileInfo.ts'). I'm not sure how this pans out in a OS with case sensitive filenames.

@pranavkm
Copy link
Contributor Author

Merged in changeset 2dc1c5b

@pranavkm pranavkm closed this Nov 14, 2012
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

4 participants