Skip to content

Some suggested changes#59

Open
TrevorPilley wants to merge 4 commits intonager:mainfrom
TrevorPilley:main
Open

Some suggested changes#59
TrevorPilley wants to merge 4 commits intonager:mainfrom
TrevorPilley:main

Conversation

@TrevorPilley
Copy link

Hi, I was playing about with your project and have a few suggestions, here's a PR in case you're interested in any of them.

This will prioritise the .NET 10 build making it easier to contribute to the library on non Windows devices.
This allows a single method definition to work for all targeted frameworks.
Although the classes doesn't allow the dictionaries to be modified after construction, this is a more formal way of declaring the intent.
The currencies have no mutable state but there are multiple instances of some created so using singletons will reduce the memory footprint of the library at runtime.
@tinohager
Copy link
Member

Thanks for the input, there are some interesting and good approaches there. Is it okay with you if I incorporate some of your ideas? I don't want to include the entire pull request as a whole at this point. Or would you like to split it into individual pull requests?

@TrevorPilley
Copy link
Author

Of course, feel free to use as much or as little of the ideas as you want. If you don’t want to take all of it, would the easiest option be for you to make a separate branch and I can redirect the PR to that instead of individual PRs at which point you could pick and choose what you like from there?

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