search

codinguser / gnucash-android

Gnucash for Android mobile companion application.
Apache License 2.0
1.23k stars 540 forks source link

Latest commodities #739

Closed pnemonic78 closed 4 years ago

pnemonic78 commented 7 years ago

fixes #731

Copied the latest currencies XML file from the GnuCash desktop version with correct IRR symbol. This requires a re-population of the commodities database table.

codinguser commented 6 years ago

Thanks for the PR @pnemonic78 I have a couple of remarks:

  1. I would prefer to do a database migration only in develop branch as far as is possible. Hotfix db migrations should only be for burning issues.
  2. Please in the future separate refactorings and feature/bugfix pull requests. This PR makes many modifications which are unrelated to the main goal of the PR and that makes it hard to understand where the actual fixes are.
  3. I do not see the new currencies XML in the list of changed files. Where is it saved?
  4. I also added a question to the PR itself (you should have gotten a notification).

Cheers,

codinguser commented 6 years ago

Thanks for the PR @pnemonic78 I have a couple of remarks:

  1. I would prefer to do a database migration only in develop branch as far as is possible. Hotfix db migrations should only be for burning issues. Could you please reissue this against develop?
  2. Please in the future separate refactorings and feature/bugfix pull requests. This PR makes many modifications which are unrelated to the main goal of the PR and that makes it hard to understand where the actual fixes are.
  3. I do not see the new currencies XML in the list of changed files. Where is it saved?
  4. I also added a question to the PR itself (you should have gotten a notification).

Cheers,

pnemonic78 commented 6 years ago
  1. will re-do in develop
  2. ok, next time.
  3. https://github.com/codinguser/gnucash-android/pull/739/commits/c3754782330cb0f44ce1c93317d16631f3134b82
  4. answered above.
pnemonic78 commented 6 years ago

ready for approval :-)

codinguser commented 6 years ago

Just to note that I didn't forget this. I will merge it into develop for the release after 2.4.0 (currently in beta)