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

Rename ref property to reference #67

Merged
merged 1 commit into from
Aug 31, 2014
Merged

Rename ref property to reference #67

merged 1 commit into from
Aug 31, 2014

Conversation

mathiasbynens
Copy link
Collaborator

Ref. #57.

@mathiasbynens
Copy link
Collaborator Author

@jviereck After this is merged, could you bump the version number again please? Then regjsgen can be updated accordingly and publish a new release as well.

@jviereck
Copy link
Owner

Good catch! Now that I see this PR, how about instead of

{
  "type": "reference",
  "reference": 1
}

do

{
  "type": "reference",
  "matchIndex": 1
}

Reasoning:

  1. get away with the duplication "reference" there
  2. just calling the property "reference=1" has low semantics. What is referred? Calling it "matchIndex" makes it a little bit more clear in my opinion.

I don't have strong opinions on this and either solution is fine for me. Thought it's worth bringing it up here as it came to my mind when reviewing the PR.

@mathiasbynens
Copy link
Collaborator Author

@jviereck Agreed. Patch updated.

@jviereck
Copy link
Owner

Wow, that was fast 👍

Sorry to spot this just now, but I guess for consistency, the variable on the function name should then also be renamed? From

function createReference(reference) {

to

function createReference(matchIndex) {

Let me know what you think. Happy to do the change myself if you agree on the change as it's my fault i didn't spot this from the beginnig :/

@mathiasbynens
Copy link
Collaborator Author

No worries. Updated the patch. Oh, and I should’ve spotted this during review in the first place!

jviereck added a commit that referenced this pull request Aug 31, 2014
Rename `ref` property to `reference`
@jviereck jviereck merged commit cf5e6f5 into master Aug 31, 2014
@jviereck
Copy link
Owner

okay, I tried to automate the release process to NPM, but failed. Seems like it's possible to configure travis to push a new version to npm following the instructions in 1. I did this and updated the .travis.yml file in the repo according this the documentation. However, when tagging the new release, I get this at the end of the travis run:

https://travis-ci.org/jviereck/regjsparser/jobs/34053640#L593

Skipping deployment with the npm provider because this branch is not permitted to deploy

Any idea what I did wrong here?


To not block the new release, I published the 0.1.2 release to npm manually.

@mathiasbynens
Copy link
Collaborator Author

I made some changes based on my interpretation of that document: 88dfcee Not sure if it works though. Let’s test it when the next version gets tagged :)

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.

2 participants