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

Issue #13 add support version #23

Merged
merged 9 commits into from
Jun 18, 2020

Conversation

melhirech
Copy link

@melhirech melhirech commented Jun 17, 2020

Fixes #13

  • Add dmm --version functionality (can be tested by doing deno run mod.ts --version)

  • Update helpMessage inside src/options/help.ts to include this new functionality

  • Update README to include this

  • Update DEV.md to have that version string updated on a new release

  • Add tests at tests/options/version_test.ts to assert it works correctly

  • Use regex to check version value string in version_test.ts

Copy link
Member

@Guergeiro Guergeiro 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!

Do you think you can do what I suggested? If so, I think you can do it in pretty much all our repositories. Just need to open an issue 😃

Comment on lines 19 to 22
assertEquals(
stdout,
"dmm 1.0.5\n",
);
Copy link
Member

Choose a reason for hiding this comment

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

I'm skeptical about hardcoded version assertions... That's something we even removed from Drash because it's mehhhh...

I might have a suggestion for this (haven't talked to @ebebbington or @crookse about it). Instead of asserting a specific version, we assert:

  1. Output is not empty.
  2. Output follows a regex that matches a valid semver string. Link to official regex here.

Honestly, if we assert number 2, we don't have to assert 1 since that case is covered in 2.

Copy link
Member

@ebebbington ebebbington Jun 18, 2020

Choose a reason for hiding this comment

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

@Guergeiro that's a good idea actually

I think the following regex would suffice: /dmm(v)?[0-9].+[0-9].+[0-9]/g; (which is actually used in dmm to get imported versions)

Copy link
Member

Choose a reason for hiding this comment

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

I don't see a problem in using the official regex though. They cover alpha releases, release candidates, etc. We literally only have to copy 😆

Copy link
Author

Choose a reason for hiding this comment

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

@Guergeiro The regex approach sound seems good to me, I'll add the official regex since it covers all possible cases.

Copy link
Member

Choose a reason for hiding this comment

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

Let’s make sure we add a comment stating where we got the regex. I like to know where we get code from when it’s copied.

Copy link
Author

@melhirech melhirech Jun 18, 2020

Choose a reason for hiding this comment

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

https://regexr.com/39s32

/^((([0-9]+)\.([0-9]+)\.([0-9]+)(?:-([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?)(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?)$/

What do you guys think about this? I guess this should do the work.

EDIT:
I edited the regex to include dmm.
const regex = /^((?:dmm)?\s)((([0-9]+)\.([0-9]+)\.([0-9]+)(?:-([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?)(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?)$/

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if it was done separately. Pseudocode:

const reg = /^((([0-9]+)\.([0-9]+)\.([0-9]+)(?:-([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?)(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?)$/;
const variable = "dmm 1.5.5";
const splittedString = variable.split(" ");
assertTrue(splittedString.length == 2);
assertEquals(splittedString[0], "dmm");
assertTrue(reg.test(splittedString[1]);

Copy link
Member

@Guergeiro Guergeiro left a comment

Choose a reason for hiding this comment

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

Good job! 🚀

@Guergeiro Guergeiro merged commit c83657a into drashland:master Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add Support for --version
4 participants