-
Notifications
You must be signed in to change notification settings - Fork 5
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
All regular issue copper and silver US coins now included. Modern pro… #190
base: main
Are you sure you want to change the base?
Conversation
…ofs now included. Improvements to existing classes which may or may not survive backup/restore. Existing classes kept for restore backward compatability although they have been added into new classes.
app/src/main/java/com/spencerpages/collections/CladDimes.java
Dismissed
Show dismissed
Hide dismissed
app/src/main/java/com/spencerpages/collections/CladKennedy.java
Dismissed
Show dismissed
Hide dismissed
app/src/main/java/com/spencerpages/collections/EarlyQuarters.java
Dismissed
Show dismissed
Hide dismissed
app/src/main/java/com/spencerpages/collections/SilverHalfDollars.java
Dismissed
Show dismissed
Hide dismissed
app/src/main/java/com/spencerpages/collections/SmallCents.java
Dismissed
Show dismissed
Hide dismissed
app/src/main/java/com/spencerpages/collections/SmallDollars.java
Dismissed
Show dismissed
Hide dismissed
app/src/main/java/com/spencerpages/collections/moreNickels.java
Dismissed
Show dismissed
Hide dismissed
Thanks for the PR - looks like some great additions! Do any of the new coin images added need attributions added in the App Info section of the app? It looks like some of the unit tests are failing. Are you familiar with those? I can help debug and fix if needed. It'd be great if we could extend our existing unit tests to the new coin sets. Let me know if you have bandwidth for that. If not, we can open a follow-on work item for those. |
Thank You for acknowledging my pull request. I now know I did it properly. All image files are from wikimedia. Most are NNC. Attribution strings were added and I believe show on the Info page. I don`t know what a unit test is but I am not adverse to learning. I don The app, as I modified it, compiles fine in android studio and runs fine on all three of my devices. Android studio does complain about unboxed primitives (allowed), spelling , and lack of international language specifications in string.format expressions (allowed but not recommended). I believe all other errors have been fixed although it is possible some were introduced when I copied my edits into the fork as I only checked that it compiled. Please note that many of your classes have had several new images added as well as the ability to add type coins and proofs but much of this gets clobbered with a restore thus these classes have been added into new classes. The original classes have been kept for backwards compatibility with restore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a first pass review - thanks for the hard work on this! There are a few more things that need fixing, mainly related to keeping existing collections working correctly. Our unit tests are failing because of that. I can work with you get those all passing though
app/src/main/java/com/spencerpages/collections/CladKennedy.java
Outdated
Show resolved
Hide resolved
app/src/main/java/com/spencerpages/collections/moreNickels.java
Outdated
Show resolved
Hide resolved
A few of the unit tests are needed because a change is needed to this line: "Nickels" needs to change to "Jefferson Nickels" (Note currently the code has "JeffersonNickels" but I left a comment suggesting that change to "Jefferson Nickels") coin-collection-android-US/shared-test/src/main/java/com/spencerpages/SharedTest.java Line 47 in f32ea27
If you want to run the unit tests locally, you can do this - Right click and select either 'Run' or 'Debug': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a second pass of review - can you take a look? Let me know if you have any questions. Thanks!
if (oldVersion <= 20) { | ||
// Add in new 2024 coins if applicable | ||
total += DatabaseHelper.addFromYear(db, collectionListInfo, 2024); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a bad Git merge - add these lines back so that existing collections (pre-2024) have the 2024 coins added
if (oldVersion <= 20) { | ||
// Add in new 2024 coins if applicable | ||
ArrayList<String> newCoinIdentifiers = new ArrayList<>(); | ||
newCoinIdentifiers.add("Rev. Dr. Pauli Murray"); | ||
newCoinIdentifiers.add("Patsy Takemoto Mink"); | ||
newCoinIdentifiers.add("Dr. Mary Edwards Walker"); | ||
newCoinIdentifiers.add("Celia Cruz"); | ||
newCoinIdentifiers.add("Zitkala-Ša"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad git merge - Add these lines back and include " 2024". I'm still thinking about that part - we may need some additional code to add the years for existing collections. I might propose that we open a follow-on task to add the years and keep them without for this PR.
{"Rev. Dr. Pauli Murray", R.drawable.women_2024_pauli_murray_unc}, | ||
{"Patsy Takemoto Mink", R.drawable.women_2024_patsy_takemoto_unc}, | ||
{"Dr. Mary Edwards Walker", R.drawable.women_2024_mary_edwards_walker_unc}, | ||
{"Celia Cruz", R.drawable.women_2024_celia_cruz_unc}, | ||
{"Zitkala-Ša", R.drawable.women_2024_zitkala_sa_unc} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for these - these got dropped and need to be added back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aDesertRat Reminder to look at this one - looks like you're missing the ones after Maria Tallchief, so:
{"Rev. Dr. Pauli Murray", R.drawable.women_2024_pauli_murray_unc},
{"Patsy Takemoto Mink", R.drawable.women_2024_patsy_takemoto_unc},
{"Dr. Mary Edwards Walker", R.drawable.women_2024_mary_edwards_walker_unc},
{"Celia Cruz", R.drawable.women_2024_celia_cruz_unc},
{"Zitkala-Ša", R.drawable.women_2024_zitkala_sa_unc}
{"Edith Kanaka'ole 2023", R.drawable.women_2023_edith_kanakaole_unc}, | ||
{"Eleanor Roosevelt 2023", R.drawable.women_2023_eleanor_roosevelt_unc}, | ||
{"Jovita Idar 2023", R.drawable.women_2023_jovita_idar_unc}, | ||
{"Maria Tallchief 2023", R.drawable.women_2023_maria_tallchief_unc}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the 2024 images here? You should be able to copy them from the AmericanWomenQuarters class
{"Flowing Hair", R.drawable.a1795_half_dollar_obv}, | ||
{"Draped Bust", R.drawable.a1796_half_dollar_obverse_15_stars}, | ||
{"Liberty Seated", R.drawable.a1885_half_dollar_obv}, | ||
{"`Liberty Seated", R.drawable.anostarsdime}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the meaning of the tick mark ( ` )?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its a different coin identifier. shows a different Liberty Seated image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let me think about this more. There may be a way we can differentiate the image based on the mint mark instead of having to use an altered name
if (oldVersion <= 20) { | ||
// Add in new 2024 coins if applicable | ||
total += DatabaseHelper.addFromYear(db, collectionListInfo, 2024); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add this back in
public int getCoinImageIdentifier() {return REVERSE_IMAGE;} | ||
|
||
private static final Integer START_YEAR = 1950; | ||
private static final Integer STOP_YEAR = 2024; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set this to STILL_IN_PRODUCTION so that it updates automatically every year
private static final Integer STOP_YEAR = 2024; | |
private static final Integer STOP_YEAR = CoinPageCreator.OPTVAL_STILL_IN_PRODUCTION; |
if (oldVersion <= 20) { | ||
// Add in new 2024 coins if applicable | ||
total += DatabaseHelper.addFromYear(db, collectionListInfo, 2024); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add this back
…t. fixed spelling of peace. reworked sm dollars to facilitate updates.
…lt dimes, kennedy halfs, and silver eagles.deleted duplicate lines from lincoln cents.renamed clad kennedy to Clad Kennedy Half Dollars and washington silver to Washington Silver Quarters as requested. fixed include strings and attr string as requested. Declined to change large cents. no images available at this time to fix 2024 quarter updates.
app/src/main/java/com/spencerpages/collections/AmericanEagleSilverDollars.java
Show resolved
Hide resolved
app/src/main/java/com/spencerpages/collections/CladKennedy.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed a few more things
new FirstSpouseGoldCoins(), | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like FirstSpouseGoldCoins() got duplicated in here (it's on line 114)
new FirstSpouseGoldCoins(), |
/* | ||
* Coin Collection, an Android app that helps users track the coins that they've collected | ||
* Copyright (C) 2010-2016 Andrew Williams | ||
* | ||
* This file is part of Coin Collection. | ||
* | ||
* Coin Collection is free software: you can redistribute it and/or modify | ||
* it under the terms of the GNU General Public License as published by | ||
* the Free Software Foundation, either version 3 of the License, or | ||
* (at your option) any later version. | ||
* | ||
* Coin Collection is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
* GNU General Public License for more details. | ||
* | ||
* You should have received a copy of the GNU General Public License | ||
* along with Coin Collection. If not, see <http://www.gnu.org/licenses/>. | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicated - should be deleted
/* | |
* Coin Collection, an Android app that helps users track the coins that they've collected | |
* Copyright (C) 2010-2016 Andrew Williams | |
* | |
* This file is part of Coin Collection. | |
* | |
* Coin Collection is free software: you can redistribute it and/or modify | |
* it under the terms of the GNU General Public License as published by | |
* the Free Software Foundation, either version 3 of the License, or | |
* (at your option) any later version. | |
* | |
* Coin Collection is distributed in the hope that it will be useful, | |
* but WITHOUT ANY WARRANTY; without even the implied warranty of | |
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | |
* GNU General Public License for more details. | |
* | |
* You should have received a copy of the GNU General Public License | |
* along with Coin Collection. If not, see <http://www.gnu.org/licenses/>. | |
*/ |
{"Flowing Hair", R.drawable.a1795_half_dollar_obv}, | ||
{"Draped Bust", R.drawable.a1796_half_dollar_obverse_15_stars}, | ||
{"Liberty Seated", R.drawable.a1885_half_dollar_obv}, | ||
{"`Liberty Seated", R.drawable.anostarsdime}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let me think about this more. There may be a way we can differentiate the image based on the mint mark instead of having to use an altered name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use the Android Studio 'Refactor' button to rename this to AllNickels.java? Right click on the file in the left hand menu, select Refactor, then Rename, then type in AllNickels
and click Refactor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aDesertRat Can you give this a try? Rename the class to AllNickels and the file name to AllNickels.java
… Added 2024 coins to Clad Quarters and Washington Silver Quarters. Added 2024 coin update to American Women Quarters (please check as I don`t understand this code).
@aDesertRat do you have some time to try this? We'll need to get our unit tests passing to complete this PR. There are a few more open PR comments that need attention as well. If you need help, I think we can merge this into a side branch in this repo and I can push fixes to that branch as well |
…Type Nickels to allow restore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting back to this now - thanks for the updates so far!
{"Rev. Dr. Pauli Murray", R.drawable.women_2024_pauli_murray_unc}, | ||
{"Patsy Takemoto Mink", R.drawable.women_2024_patsy_takemoto_unc}, | ||
{"Dr. Mary Edwards Walker", R.drawable.women_2024_mary_edwards_walker_unc}, | ||
{"Celia Cruz", R.drawable.women_2024_celia_cruz_unc}, | ||
{"Zitkala-Ša", R.drawable.women_2024_zitkala_sa_unc} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aDesertRat Reminder to look at this one - looks like you're missing the ones after Maria Tallchief, so:
{"Rev. Dr. Pauli Murray", R.drawable.women_2024_pauli_murray_unc},
{"Patsy Takemoto Mink", R.drawable.women_2024_patsy_takemoto_unc},
{"Dr. Mary Edwards Walker", R.drawable.women_2024_mary_edwards_walker_unc},
{"Celia Cruz", R.drawable.women_2024_celia_cruz_unc},
{"Zitkala-Ša", R.drawable.women_2024_zitkala_sa_unc}
if (oldVersion <= 2) { | ||
// Need to add in 1968 - 1970 for Half Dollars unless it doesn't exist | ||
// NOTE - We can't fix this, because adding will mess up the _id fields, which we use to do ordering | ||
// We could make a new table and copy over all of the data, but that presents a lot of challenges | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aDesertRat Reminder for this one - Need to add these lines back in:
if (oldVersion <= 2) {
// Need to add in 1968 - 1970 for Half Dollars unless it doesn't exist
// NOTE - We can't fix this, because adding will mess up the _id fields, which we use to do ordering
// We could make a new table and copy over all of the data, but that presents a lot of challenges
}
{"Flowing Hair Chain Reverse", R.drawable.annc_us_1793_1c_flowing_hair_cent}, | ||
{"Flowing Hair Wreath Reverse", R.drawable.annc_us_1793_1c_flowing_hair_cent}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aDesertRat Any thoughts on this one? Should we keep Reverse in the title or remove it?
coinList.add(new CoinSlot("Flowing Hair Chain Reverse", date, coinIndex++)); | ||
coinList.add(new CoinSlot("Flowing Hair Wreath Reverse", date, coinIndex++));} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aDesertRat Same for this one
/* | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aDesertRat Delete these 3 lines - must have been copied in by mistake
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aDesertRat Can you give this a try? Rename the class to AllNickels and the file name to AllNickels.java
newCoinIdentifiers.add("Dr. Mary Edwards Walker"); | ||
newCoinIdentifiers.add("Celia Cruz"); | ||
newCoinIdentifiers.add("Zitkala-Ša"); | ||
newCoinIdentifiers.add("Rev. Dr. Pauli Muarray 2024"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Muarray should be Murray
newCoinIdentifiers.add("Celia Cruz"); | ||
newCoinIdentifiers.add("Zitkala-Ša"); | ||
newCoinIdentifiers.add("Rev. Dr. Pauli Muarray 2024"); | ||
newCoinIdentifiers.add("Patsy Takemoto 2024"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one should still be Patsy Takemoto Mink
to match the mint website
newCoinIdentifiers.add("Patsy Takemoto 2024"); | ||
newCoinIdentifiers.add("Dr. Mary Edwards Walker 2024"); | ||
newCoinIdentifiers.add("Celia Cruz 2024"); | ||
newCoinIdentifiers.add("Zitkala Sa 2024"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep this one consistent with the mint also Zitkala-Ša
@aDesertRat One option that may make things easier to wrap this PR up is to break it into two. All of the new collections can be added without any worry about breaking existing collections, so we could move those to a separate branch and PR, and get those merge in first. Then we'd have the updates to existing collections, and the updates to unit tests to worry about. Any thoughts? Are you familiar with Git and making new branches? Git makes it pretty easy to pull changes from one branch to another. |
If you wish to cherry pick the PR, well, your the maintainer and I provide these files without reservation. You may wish to wait awhile until I perfect edits to hide labels etc and add mintages. Please do keep the ability to add type coins. A typical wheat cent collector, for example, may have most of the Lincolns, a few Indian Heads, and a large cent or two and find it useful to view them all as one collection rather than adding two mostly empty collections. As far as the remaining unit test failures go I consider them to be trivial and safely ignored limitations in the test code (or user error) except for the American Women class update which really is broken and always has been. For my own use, I plan to go the other way and hide (keeping restore ability) most of the existing classes which I consider to be incomplete and obsolete. At any rate it's been a useful learning experience. Thank You.
Sent with [Proton Mail](https://proton.me/mail/home) secure email.
…On Wednesday, January 22nd, 2025 at 1:37 AM, Spencer Williams ***@***.***> wrote:
***@***.***(https://github.com/aDesertRat) One option that may make things easier to wrap this PR up is to break it into two. All of the new collections can be added without any worry about breaking existing collections, so we could move those to a separate branch and PR, and get those merge in first. Then we'd have the updates to existing collections, and the updates to unit tests to worry about. Any thoughts? Are you familiar with Git and making new branches? Git makes it pretty easy to pull changes from one branch to another.
—
Reply to this email directly, [view it on GitHub](#190 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/BJWPVS4N4PYJOTMNKL3DFW32L5KEDAVCNFSM6AAAAABUXA5DM6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMBWGYYDANJYGU).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
@aDesertRat That sounds good! I am not an experienced maintainer so still learning that part. I created this branch/PR (#195) which pulls the new collections from this PR, so keeps all existing collections and tests passing. I believe I have done everything correctly to where I'll merge that (w/ rebase), and it will correctly attribute the new additions to you. Once that's merged, we can merge this PR with main, resolve the conflicts (I can help with that part), and continue working on this PR for existing collection updates or test updates. It should make this PR smaller and make the page load a little easier as well. I am also planning to make an update that adds an optional "image id" for each coin slot, which will allow the collection creation (or user) to specify the coin image without relying on the name alone. You'll be able to use that instead of having to rely on the name being "", " ", or "`". We can switch to that mechanism before we make the next app release. Let me know if you see any issues with this plan, or if you'd prefer to wait and get everything in on this PR. Thanks! |
Sounds good.. I see there already is a function obverse image collected that i was planning to exploit or mimic for a image identifier. No doubt you can do it better as I'm not really a java programmer. I am in the process of going through each coin type in each class and creating a collection for each mint variety and comparing them to online lists. While the full collections list properly, I have made code adjustments so mint varieties do. So far I have done cents, nickels, and dimes. I will switch to my "Early" classes since the test classes are mostly full (true?) and I am guessing these are the classes you want.
Sent with [Proton Mail](https://proton.me/mail/home) secure email.
…On Friday, January 24th, 2025 at 1:37 AM, Spencer Williams ***@***.***> wrote:
> If you wish to cherry pick the PR, well, your the maintainer and I provide these files without reservation. You may wish to wait awhile until I perfect edits to hide labels etc and add mintages. Please do keep the ability to add type coins. A typical wheat cent collector, for example, may have most of the Lincolns, a few Indian Heads, and a large cent or two and find it useful to view them all as one collection rather than adding two mostly empty collections. As far as the remaining unit test failures go I consider them to be trivial and safely ignored limitations in the test code (or user error) except for the American Women class update which really is broken and always has been. For my own use, I plan to go the other way and hide (keeping restore ability) most of the existing classes which I consider to be incomplete and obsolete. At any rate it's been a useful learning experience. Thank You. Sent with [Proton Mail](https://proton.me/mail/home) secure email.
> […](#)
> On Wednesday, January 22nd, 2025 at 1:37 AM, Spencer Williams @.> wrote: @.(https://github.com/aDesertRat) One option that may make things easier to wrap this PR up is to break it into two. All of the new collections can be added without any worry about breaking existing collections, so we could move those to a separate branch and PR, and get those merge in first. Then we'd have the updates to existing collections, and the updates to unit tests to worry about. Any thoughts? Are you familiar with Git and making new branches? Git makes it pretty easy to pull changes from one branch to another. — Reply to this email directly, [view it on GitHub]([#190 (comment)](#190 (comment))), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/BJWPVS4N4PYJOTMNKL3DFW32L5KEDAVCNFSM6AAAAABUXA5DM6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMBWGYYDANJYGU). You are receiving this because you were mentioned.Message ID: @.***>
***@***.***(https://github.com/aDesertRat) That sounds good! I am not an experienced maintainer so still learning that part.
I created this branch/PR ([#195](#195)) which pulls the new collections from this PR, so keeps all existing collections and tests passing. I believe I have done everything correctly to where I'll merge that (w/ rebase), and it will correctly attribute the new additions to you. Once that's merged, we can merge this PR with main, resolve the conflicts (I can help with that part), and continue working on this PR for existing collection updates or test updates. It should make this PR smaller and make the page load a little easier as well.
I am also planning to make an update that adds an optional "image id" for each coin slot, which will allow the collection creation (or user) to specify the coin image without relying on the name alone. You'll be able to use that instead of having to rely on the name being "", " ", or "`". We can switch to that mechanism before we make the next app release.
Let me know if you see any issues with this plan, or if you'd prefer to wait and get everything in on this PR. Thanks!
—
Reply to this email directly, [view it on GitHub](#190 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/BJWPVSZAXSFB7EUGVMBFPDT2MH3S3AVCNFSM6AAAAABUXA5DM6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMJRHE3DCNZWGU).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
PR 195
Sent with [Proton Mail](https://proton.me/mail/home) secure email.
I have reviewed PR 195. None of the commits of edits to address your or my concerns seem to be included. Android studio is unable to find this repository so I can't edit it. The link works so I can view it. I'm guessing I don't have permissions to edit. Do I add edits to my repository and do another PR? How does this work?
…On Friday, January 24th, 2025 at 1:37 AM, Spencer Williams ***@***.***> wrote:
> If you wish to cherry pick the PR, well, your the maintainer and I provide these files without reservation. You may wish to wait awhile until I perfect edits to hide labels etc and add mintages. Please do keep the ability to add type coins. A typical wheat cent collector, for example, may have most of the Lincolns, a few Indian Heads, and a large cent or two and find it useful to view them all as one collection rather than adding two mostly empty collections. As far as the remaining unit test failures go I consider them to be trivial and safely ignored limitations in the test code (or user error) except for the American Women class update which really is broken and always has been. For my own use, I plan to go the other way and hide (keeping restore ability) most of the existing classes which I consider to be incomplete and obsolete. At any rate it's been a useful learning experience. Thank You. Sent with [Proton Mail](https://proton.me/mail/home) secure email.
> […](#)
> On Wednesday, January 22nd, 2025 at 1:37 AM, Spencer Williams @.> wrote: @.(https://github.com/aDesertRat) One option that may make things easier to wrap this PR up is to break it into two. All of the new collections can be added without any worry about breaking existing collections, so we could move those to a separate branch and PR, and get those merge in first. Then we'd have the updates to existing collections, and the updates to unit tests to worry about. Any thoughts? Are you familiar with Git and making new branches? Git makes it pretty easy to pull changes from one branch to another. — Reply to this email directly, [view it on GitHub]([#190 (comment)](#190 (comment))), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/BJWPVS4N4PYJOTMNKL3DFW32L5KEDAVCNFSM6AAAAABUXA5DM6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMBWGYYDANJYGU). You are receiving this because you were mentioned.Message ID: @.***>
***@***.***(https://github.com/aDesertRat) That sounds good! I am not an experienced maintainer so still learning that part.
I created this branch/PR ([#195](#195)) which pulls the new collections from this PR, so keeps all existing collections and tests passing. I believe I have done everything correctly to where I'll merge that (w/ rebase), and it will correctly attribute the new additions to you. Once that's merged, we can merge this PR with main, resolve the conflicts (I can help with that part), and continue working on this PR for existing collection updates or test updates. It should make this PR smaller and make the page load a little easier as well.
I am also planning to make an update that adds an optional "image id" for each coin slot, which will allow the collection creation (or user) to specify the coin image without relying on the name alone. You'll be able to use that instead of having to rely on the name being "", " ", or "`". We can switch to that mechanism before we make the next app release.
Let me know if you see any issues with this plan, or if you'd prefer to wait and get everything in on this PR. Thanks!
—
Reply to this email directly, [view it on GitHub](#190 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/BJWPVSZAXSFB7EUGVMBFPDT2MH3S3AVCNFSM6AAAAABUXA5DM6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMJRHE3DCNZWGU).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
@aDesertRat Ah ok - first, I can grab the latest updates from this PR to see if I missed anything. Then if you see anything I missed or want to push additional changes, run these commands: In your forked repo run:
|
@aDesertRat FYI - I've added the ability to specify images for each coin using an image id. I've created a PR with that, here: I'll merge this and will update #195 I can also do the merge and conflict resolution on this branch/PR if you'd like. It says I have push access, so I can try it if you'd like. Not sure how familiar you are with merging and resolving conflicts |
Pass criteria - unit test numbers add up - comparison to online database total Half Cent 44 no paramaters pass Large Cents 68 bust-25 coronet-43 pass Two Cents 10 no parameters pass Trimes 49 silver-24 nickel-25 pass Twenty Cents 7 no parameters pass total P O S CC D Half Dimes 96 pass bust 18 pass no mm seated 78 45 21 12 0 0 pass Early Dimes 144 pass draped 10 pass no mm capped 20 pass no mm seated 114 57 19 29 9 0 pass Early Quarters 244 pass bust 24 pass no mm seated 109 56 20 24 9 0 pass barber 74 25 18 21 0 10 pass standing 37 15 0 12 0 10 pass Early Halfs 158 pass bust 44 pass contains two O mm coins seated 114 55 23 26 10 -- pass Early Dollars 86 pass bust 13 pass no mm seated 49 36 5 4 4 0 pass trade 24 12 0 6 6 0 pass
I'm lost when it comes to git. I still haven't figured out how to edit 195. Image editing - very nice but I can't use it yet. I have, however, created collections on my device for classes, subclasses, and mint marks. I had to edit some classes to make the mint mark collections display properly. I like the labels in these classes but see the value in image editing and would like to include it. I compared these collections with a online database to make sure they were complete. It is all included in my latest commit to my github repository. I would like to commit it to 195 but don't know how. Here are the numbers and classes in question.
Half Cent 44 no paramaters pass
Large Cents 68 bust-25 coronet-43 pass
Two Cents 10 no parameters pass
Trimes 49 silver-24 nickel-25 pass
Twenty Cents 7 no parameters pass
total P O S CC D
Half Dimes 96 pass
bust 18 pass no mm creator
seated 78 45 21 12 0 0 pass
Early Dimes 144 pass
draped 10 pass no mm creator
capped 20 pass no mm creator
seated 114 57 19 29 9 0 pass
Early Quarters 244 pass
bust 24 pass no mm creator
seated 109 56 20 24 9 0 pass
barber 74 25 18 21 0 10 pass
standing 37 15 0 12 0 10 pass
Early Halfs 158 pass
bust 44 42 2 pass
seated 114 55 23 26 10 -- pass
Early Dollars 86 pass
bust 13 pass no mm creator
seated 49 36 5 4 4 0 pass trade 24 12 0 6 6 0 pass
Sent with [Proton Mail](https://proton.me/mail/home) secure email.
…On Sunday, January 26th, 2025 at 12:32 PM, Spencer Williams ***@***.***> wrote:
***@***.***(https://github.com/aDesertRat) FYI - I've added the ability to specify images for each coin using an image id. I've created a PR with that, here:
[#196](#196)
I'll merge this and will update [#195](#195)
I can also do the merge and conflict resolution on this branch/PR if you'd like. It says I have push access, so I can try it if you'd like. Not sure how familiar you are with merging and resolving conflicts
—
Reply to this email directly, [view it on GitHub](#190 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/BJWPVS2EKIEKXBNFXSKX6FL2MUZ4PAVCNFSM6AAAAABUXA5DM6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMJUGU3DCNZRGM).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
@aDesertRat I think I'm missing the necessary permissions to let you edit the 195 branch directly, so I will instead pull your latest updates from this branch and merge them into that one. Once I'm done, I'll have you review that PR one last time and you can leave any comments. I'm hoping to carve out some time today or tomorrow to get your changes merged in |
…pass unit test. Created option to create clad,silver, or combined collections. Deleted no longer needed cladRoosevelt and cladKennedy classes. Unit test results: total P D S Satin Proof unit inspecion Roosevelt Dimes 285 88 82 10 12 105 pass pass no types clad 189 69 63 0 12 57 pass pass silver 96 19 19 10 0 48 pass pass Kennedy Halfs 221 60 57 0 12 92 pass pass no types clad 171 53 53 0 12 53 pass pass silver 50 7 4 o 39 pass pass Eisenhower Dol 32 8 8 silver-10 6 pass pass
It looks like you have done the merge -- Thank You. I only see a few things that could be changed. On line 119 of CladQuarters there is a typo, 2012 should be 2010. Mint Sets and Proof Sets are obsoleted by Sets and could be deleted. And finally in small cents the 1982 cents break unit tests and display unwanted in several configurations. I have a commit that fixes that (third times a charm!).
I see now that I should have done several much smaller PR's as one big PR has been a hassle for both of us. Quality control was also a learning experience and should have been done before the PR. Thank you for your patience.
Sent with [Proton Mail](https://proton.me/mail/home) secure email.
…On Wednesday, January 29th, 2025 at 6:33 PM, Spencer Williams ***@***.***> wrote:
> I'm lost when it comes to git. I still haven't figured out how to edit 195. Image editing - very nice but I can't use it yet. I have, however, created collections on my device for classes, subclasses, and mint marks. I had to edit some classes to make the mint mark collections display properly. I like the labels in these classes but see the value in image editing and would like to include it. I compared these collections with a online database to make sure they were complete. It is all included in my latest commit to my github repository. I would like to commit it to 195 but don't know how. Here are the numbers and classes in question. Half Cent 44 no paramaters pass Large Cents 68 bust-25 coronet-43 pass Two Cents 10 no parameters pass Trimes 49 silver-24 nickel-25 pass Twenty Cents 7 no parameters pass total P O S CC D Half Dimes 96 pass bust 18 pass no mm creator seated 78 45 21 12 0 0 pass Early Dimes 144 pass draped 10 pass no mm creator capped 20 pass no mm creator seated 114 57 19 29 9 0 pass Early Quarters 244 pass bust 24 pass no mm creator seated 109 56 20 24 9 0 pass barber 74 25 18 21 0 10 pass standing 37 15 0 12 0 10 pass Early Halfs 158 pass bust 44 42 2 pass seated 114 55 23 26 10 -- pass Early Dollars 86 pass bust 13 pass no mm creator seated 49 36 5 4 4 0 pass trade 24 12 0 6 6 0 pass Sent with [Proton Mail](https://proton.me/mail/home) secure email.
> […](#)
> On Sunday, January 26th, 2025 at 12:32 PM, Spencer Williams @.> wrote: @.(https://github.com/aDesertRat) FYI - I've added the ability to specify images for each coin using an image id. I've created a PR with that, here: #196 I'll merge this and will update #195 I can also do the merge and conflict resolution on this branch/PR if you'd like. It says I have push access, so I can try it if you'd like. Not sure how familiar you are with merging and resolving conflicts — Reply to this email directly, [view it on GitHub]([#190 (comment)](#190 (comment))), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/BJWPVS2EKIEKXBNFXSKX6FL2MUZ4PAVCNFSM6AAAAABUXA5DM6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMJUGU3DCNZRGM). You are receiving this because you were mentioned.Message ID: @.***>
***@***.***(https://github.com/aDesertRat) I think I'm missing the necessary permissions to let you edit the 195 branch directly, so I will instead pull your latest updates from this branch and merge them into that one. Once I'm done, I'll have you review that PR one last time and you can leave any comments. I'm hoping to carve out some time today or tomorrow to get your changes merged in
—
Reply to this email directly, [view it on GitHub](#190 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/BJWPVS7GF7G67PG43I6RX4L2NF6PVAVCNFSM6AAAAABUXA5DM6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMRTGMZDGNJXGY).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
total P D S Satin Proof unit inspect Small Cents 430 178 115 53 18 66 pass pass type pass eagle pass indian 55 53 0 2 0 0 pass pass wheat 141 50 47 44 0 0 pass pass memorial 234 75 68 7 18 66 pass pass type and eagle cents N/A as they would appear in all counts
All Nickels 365 143 107 42 14 59 pass pass shield (18) pass pass liberty 33 31 1 1 0 0 pass pass buffalo 64 22 20 22 0 0 pass pass jefferson 268 90 86 19 14 59 pass pass type and shield nickels left unselected as they would appear in all counts total P D S O Proof Silver Dimes 232 71 52 59 17 33 pass barber 74 25 8 24 17 0 pass mercury 77 27 25 25 0 0 pass roosevelt 81 19 19 10 0 33 pass Roosevelt Dimes 285 88 82 10 12 105 pass pass clad 189 69 63 0 12 57 pass pass silver 96 19 19 10 0 48 pass pass type and eagle cents left unselected as they would appear in all counts Silver Halfs 228 67 46 54 32 57 pass pass barber 73 24 7 24 18 0 pass pass wakkers 65 20 21 24 0 0 pass pass franklin 35 16 14 5 0 0 pass pass kennedy 55 7 4 1 0 43 pass pass type left unselected as they would appear in all counts Kennedy Halfs 221 60 57 0 12 92 pass pass clad 171 53 53 0 12 53 pass pass silver 50 7 4 o 39 pass pass type left unselected as they would appear in all counts cc cartwheels 195 89 57 53 26 13 pass pass morgan 96 28 1 28 26 13 pass pass peace 24 10 5 9 0 0 pass pass ike 32 8 8 16 0 0 pass pass eagle 43 43 0 0 0 0 pass pass type left unselected as they would appear in all counts eagle w burnished in p. no more mint parameters for w small dollars 206 68 68 3 -- 67 pass pass sba 15 4 4 3 -- 4 pass pass native 72 24 24 0 -- 24 pass pass presidents 119 40 40 0 -- 39 pass pass Eisenhower Dol 32 8 8 silver-10 6 pass pass
…ofs now included. Improvements to existing classes which may or may not survive backup/restore. Existing classes kept for restore backward compatability although they have been added into new classes.