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

Update ShortGuid.cs #2

Merged
merged 3 commits into from
May 7, 2020
Merged

Update ShortGuid.cs #2

merged 3 commits into from
May 7, 2020

Conversation

vanillajonathan
Copy link
Contributor

Explicitly declare property visibility.
Proper capitalization of Base64.
Proper punctuation.

Explicitly declare property visibility.
Proper capitalization of Base64.
Proper punctuation.
@vanillajonathan
Copy link
Contributor Author

@davetransom ping!

@davetransom
Copy link
Member

Sorry @vanillajonathan, I forgot about this one when I didn't quite know how I should reply straight away.

The documentation corrections are worth accepting (and thank you), however, reorganising the struct is not something I'd merge as I've written it the way I want it.

I'll happily merge if you revert the stylistic changes.

@vanillajonathan
Copy link
Contributor Author

vanillajonathan commented May 7, 2020

But members of a class should be have theit member visibility explicitly declared, and if not explicitly declared as private, they are implicitly internal, and in this case I believe they should be private. Also private and internal members should be placed above public members, and non-static members should be placed above static members according to the Microsoft Framework Design Guidelines.

@davetransom
Copy link
Member

Are you confusing default access modifiers for class (internal) vs members (private)?

My previous comment still stands.

@vanillajonathan
Copy link
Contributor Author

Yes. I am confusing default access modifiers.

I updated the patch.

@davetransom davetransom merged commit 0243dab into csharpvitamins:master May 7, 2020
@vanillajonathan vanillajonathan deleted the patch-2 branch May 7, 2020 12:50
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.

None yet

2 participants