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

Which git branch should be based on when submitting a pull request #4827

Closed
thxiami opened this issue Jun 20, 2020 · 5 comments
Closed

Which git branch should be based on when submitting a pull request #4827

thxiami opened this issue Jun 20, 2020 · 5 comments
Labels

Comments

@thxiami
Copy link
Contributor

thxiami commented Jun 20, 2020

Hi, I'd eventually like to contribute in some way but I encountered a problem.

Brief of question

This is my first time participating in open source work. So I searched for the correct way to submit a PR. Then I found the most appropriate path forward would be to fork, test, and build out of one git branch(e.g. the master branch). But after reading the CONTRIBUTING.md,
I was confused about which git branch should I commit to: master or 4.17.15-post.

The ways I had tried to solve this problem:

  1. Read CONTRIBUTING.md
    In the Contributing to Lodash section I found:

Contributions are always welcome. Before contributing please read the code of conduct & search the issue tracker; your issue may have already been discussed or fixed in master. To contribute, fork Lodash, commit your changes, & send a pull request.

It seems like I should checkout a new branch based master, but then I found that in the section Pull request:

For additions or bug fixes you should only need to modify lodash.js. Include updated unit tests in the test directory as part of your pull request.
...
Before running the unit tests you’ll need to install, npm i, development dependencies. Run unit tests from the command-line via npm test, or open test/index.html & test/fp.html in a web browser.

I didn't find lodash.js under the master branch, and npm run test also didn't work because of the lack of files needed for building and testing.
But I found the lodash.js under the 4.17.15-post branch, and npm run test could work .

So, if I want to fix a bug, should I develop based on the 4.17.15-post branch?

Then I found the following paragraph in the Notice section:

Pardon the mess. The master branch is in flux while we work on Lodash v5. This means things like npm scripts, which we encourage using for contributions, may not be working. Thank you for your patience.

Does this mean contributions should not commit to the master branch for now?

  1. Search recent pull requests
    To figure it out, I checked some recent pull requests, and found that some were commited based on the master branch (PR#4681), and some were commited based on 4.17.15-post branch (PR#4515), so I was completly confused:

Detail of question

  1. If I want to fix a bug, which branch should be used for development?

  2. If I only modify a document file like REAMDE.md or CONTRIBUTING.md, which branch should be used to submit?

Hope someone can help.

@thxiami
Copy link
Contributor Author

thxiami commented Jun 20, 2020

In addition, I have encountered another problem: After cloning the code to the local, checkout the master branch, then run npm run style, some errors were prompted:

..../lodash/isFunction.js
  16:3  error  Trailing spaces not allowed  no-trailing-spaces
  19:3  error  Trailing spaces not allowed  no-trailing-spaces
  22:3  error  Trailing spaces not allowed  no-trailing-spaces
  25:3  error  Trailing spaces not allowed  no-trailing-spaces

...../lodash/unescape.js
  34:79  error  There should be no space before this paren  space-in-parens

Does that expected ? Or the managers will resolve these kind of code style problems before build?

thxiami referenced this issue in xiaoxiangmoe/lodash Jun 21, 2020
@thxiami
Copy link
Contributor Author

thxiami commented Jul 2, 2020

Is anyone could help? Thanks!

@bnjmnt4n
Copy link
Contributor

bnjmnt4n commented Jul 3, 2020

Pardon the mess. The master branch is in flux while we work on Lodash v5. This means things like npm scripts, which we encourage using for contributions, may not be working. Thank you for your patience.

Hey, as you've noticed the master branch is for lodash's next major version, while the 4.17.15-post branch are for edits to the previous version (v4), which have to be back-ported for bugfixes. So only bugfixes should move into the v4 branch, while other changes should go into master.

@bnjmnt4n bnjmnt4n closed this as completed Jul 3, 2020
@thxiami
Copy link
Contributor Author

thxiami commented Jul 3, 2020

@bnjmnt4n First of all thank you very much for your reply! And sorry to bother you again, I haven’t seen the issue closed when I write the reply. I still have a little question.

Hey, as you've noticed the master branch is for lodash's next major version, while the 4.17.15-post branch are for edits to the previous version (v4), which have to be back-ported for bugfixes. So only bugfixes should move into the v4 branch, while other changes should go into master.

I have added some code locally based on the 4.17.15-post branch, and I plan to submit two PRs later. But according to your reply above, now I'm a little confused whether they are bugfixes or just enhancements. Can you please help me judge which one they belong to?

Both PRs are related to _.clone() / _.cloneDeep()

  1. _.clone() and _.cloneDeep() don't clone the groups property of array returned by RegExp#exec .
let re = /(?<year>\d{4})-(?<month>\d{2})-(?<day>\d{2})/u;
let result = re.exec('2015-01-02');
console.log('result: \n', result)
console.log('result from shallow copy:\n', _.clone(result))
console.log('result from deep copy:\n',_.cloneDeep(result))

image

Later I will submit a PR to make _.clone() and _.cloneDeep() clone the groups property.

  1. _.cloneDeep() dosen't deep clone the key of Map
const obj = {a: 1}
const map = new Map()
map.set(obj, 1)

var deepClonedMap = _.cloneDeep(map)
deepClonedMap.get(obj) // 1, which proves that the key of deepClonedMap is still `obj`

deepClonedMap.forEach((value,key) => {
    console.log('key:', key) // {a: 1}
    console.log('key === obj:', key === obj) // true
})

Later I will submit a PR to make _.cloneDeep() deep clone the key of Map.

In your opinion, should they be bug fixes, or is it more appropriate to call them enhancements?

Thanks a lot!

@thxiami
Copy link
Contributor Author

thxiami commented Jul 7, 2020

@bnjmnt4n Sorry to bother you, could you please help me solve the above questions? Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants