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

Add display flags for version helper #36

Merged
merged 7 commits into from
Feb 14, 2017

Conversation

danielleadams
Copy link
Contributor

@danielleadams danielleadams commented Feb 9, 2017

Adds ability to remove parts of app version with flags on Helper. Defaults to displaying both app version on package.json and most recent git SHA.

Addresses #35.

Usage:

{{app-version hideSha=true}}

// => 2.0.1

{{app-version hideVersion=true}}

// => <git SHA>

TODO:

  • Update README for usage after initial approval

Copy link
Contributor

@alexlafroscia alexlafroscia left a comment

Choose a reason for hiding this comment

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

Everything looks good to me; the only thing I might change would be exporting the two regular expressions in app/helpers/app-version.js as constants, and then importing them in the tests files where they're used again, rather than repeating them.

@danielleadams
Copy link
Contributor Author

danielleadams commented Feb 10, 2017

Sounds good to me. @alexlafroscia would you suggest exporting them directly from the helper, or maybe moving them to a util?

UPDATE: Went ahead an added to a util. Let me know what you think!

@@ -0,0 +1 @@
export { default } from 'ember-cli-app-version/utils/regexp';
Copy link
Contributor

@alexlafroscia alexlafroscia Feb 12, 2017

Choose a reason for hiding this comment

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

Is it necessary to re-export these utilities in the app tree? I think that the util generator probably just created this file by default, but keeping this file here would define

import regexp from 'consuming-app/utils/regexp';

Which is probably not desired.

const versionRegExp = /\d[.]\d[.]\d/;
const shaRegExp = /[a-z\d]{8}/;

export default {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for exporting as a single object, rather than two named exports? Since all of the places that the file is imported destructures both properties anyway, it seems marginally easier to instead do:

import { shaRegExp, versionRegExp } from 'ember-cli-app-version/utils/regexp';

instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, it was moreso if this was ever needed to be exported as an object... but I guess when that case arises, it can be added.

Copy link
Contributor

@alexlafroscia alexlafroscia Feb 13, 2017

Choose a reason for hiding this comment

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

Gotcha. If you ever wanted them both as one object, you could also do

import * as regExpObject from 'ember-cli-app-version/utils/regexp';

too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! So much better.

@@ -0,0 +1,24 @@
import { module, test } from 'qunit';
import regexp from 'dummy/utils/regexp';
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note: if you remove the export of the utilities from the app tree, this will have to be updated to import from the addon, too.

@alexlafroscia
Copy link
Contributor

I pinged @taras to see if he wants to review; if he doesn't get back to me soon I'll merge your PR (and keep my fingers crossed that he doesn't mind).

@taras taras merged commit 02bd322 into ember-cli:master Feb 14, 2017
@taras
Copy link
Contributor

taras commented Feb 14, 2017

@alexlafrosia I still don't have my computer. Can you publish this?

@alexlafroscia
Copy link
Contributor

alexlafroscia commented Feb 14, 2017

I can't publish this on NPM, I don't have the permissions for it 😦

NPM says I'm part of "0 organizations" on there, and the package is published under your username. Maybe we can:

  • Create an EmberSherpa org on NPM
  • Move the package to the org
  • Add me to the org

So we can reduce the bus-factor of this addon (and your other EmberSherpa ones) in the future.

@danielleadams
Copy link
Contributor Author

@alexlafroscia @taras Any idea when this will be published?

@alexlafroscia
Copy link
Contributor

alexlafroscia commented Feb 22, 2017

I unfortunately can't publish it, even though I'd love to 😦 @taras has the keys and he's been traveling a lot lately.

I'll try to get in touch with him and see if we can transfer this repo on NPM in a way that I can help with updating going forward.

@omarkohl
Copy link

+1 for publishing this :-)

@taras
Copy link
Contributor

taras commented Mar 11, 2017

Published 2.0.2 (sorry for the delay, finally got my computer back)

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