Skip to content
This repository was archived by the owner on Apr 8, 2019. It is now read-only.

[assignment 2] Convert version route to plugin#44

Merged
hueniverse merged 1 commit into
outmoded:masterfrom
MylesBorins:assignment-2
Mar 24, 2015
Merged

[assignment 2] Convert version route to plugin#44
hueniverse merged 1 commit into
outmoded:masterfrom
MylesBorins:assignment-2

Conversation

@MylesBorins

Copy link
Copy Markdown
Contributor

Closes #43

Comment thread lib/version.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

newline after function

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

got it!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@MylesBorins
MylesBorins force-pushed the assignment-2 branch 5 times, most recently from bb0485c to 462de95 Compare March 18, 2015 17:32
Comment thread lib/version.js Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a callback so return

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call!

@hueniverse hueniverse added this to the 0.0.2 milestone Mar 18, 2015
@ai10

ai10 commented Mar 19, 2015

Copy link
Copy Markdown

Do we want to check if the version is a valid semver?

@AdriVanHoudt

Copy link
Copy Markdown
Contributor

@Sschuck the version comes from package.json and that should always follow semver, so no need to check

Comment thread lib/index.js Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is odd. Just var Version = require('./version'); at the top and then use that in register().

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated

@MylesBorins
MylesBorins force-pushed the assignment-2 branch 2 times, most recently from 8ae937a to 6961d7f Compare March 22, 2015 19:51
Comment thread lib/index.js Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No empty line here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In personal projects I have always put a new line between modules that are included from node_modules and modules that are being included locally from the project.

That aside the style has been fixed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I also like to do this, makes it easier to see what are local modules

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I just sort them: node, npm, local.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reasonable... I follow the same pattern (with spaces).

Thanks for taking the tie to discuss

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same sorting here only with newlines in between

hueniverse pushed a commit that referenced this pull request Mar 24, 2015
[assignment 2] Convert version route to plugin
@hueniverse
hueniverse merged commit fa3f5cc into outmoded:master Mar 24, 2015
hueniverse pushed a commit that referenced this pull request Mar 24, 2015
hueniverse pushed a commit that referenced this pull request Mar 24, 2015
MylesBorins pushed a commit to MylesBorins/hueniversity that referenced this pull request Mar 31, 2015
MylesBorins pushed a commit to MylesBorins/hueniversity that referenced this pull request Mar 31, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Convert to a plugin

4 participants