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

Minor message tweaks #14

Merged
merged 3 commits into from
Jun 18, 2018
Merged

Conversation

dtchepak
Copy link
Member

Some very minor message tweaks. Mainly fixing spacing after a ".", but also ended up tweaking the ctor mismatch messages as well.

@tpodolak
Copy link
Member

tpodolak commented Jun 17, 2018

Thanks for the fixes @dtchepak. Can you also fix visual basic resource file? And I think as you changed the messages the tests will be failing now

@dtchepak
Copy link
Member Author

@tpodolak Ah of course, sorry! (I just adjusted the messages via the GH online editor rather than doing it properly.) I'll resend the corrected PR once I get a chance.

@dtchepak
Copy link
Member Author

@tpodolak Think I've fixed my mistake now. 😅

That's a surprising amount of work! I'll never ask you for a message change again! 😂

Is it possible to have a shared dictionary for messages and have per-language dictionaries for just the things that vary? (virtual vs. overridable for example, or format strings for generic types as For<T> vs For(Of T)) That may cut down the work a little and also help us to keep the messages in sync (this one is quite different in the VB version for example).

It may also be worth updating the tests to also use the resource dictionary entries -- I think testing that we have the intended message defined in the resources is more important than double-checking the message wording. I think a single source-of-truth for those is ok.

I'm happy to try to migrate the tests to this approach if it sounds reasonable? I'm probably missing lots of the details sorry, so please point out any problems with this idea. :)

@dtchepak
Copy link
Member Author

dtchepak commented Jun 18, 2018

By the way, if you're happy with this, please "Squash and merge" into the original PR, then merge GH-1-SubstituteForAnalyzers too as I think it is good to go. 👍 😄

@tpodolak
Copy link
Member

Shared resource file sounds like a good idea, not sure if .net allows merging resx files, but even if it doesn't we can always "manualy" use the one we are interested in. When it comes to test changes I think we can do it as well, this will save time in the future for sure, although some might say that this is anti-pattern as stated in here https://enterprisecraftsmanship.com/2018/01/30/leaking-domain-knowledge-tests/. Even though feel free to change them, from my perspective advantages outweigh disadvantages in this case.

@tpodolak tpodolak merged commit e1317b4 into GH-1-SubstituteForAnalyzers Jun 18, 2018
@dtchepak dtchepak deleted the dtchepak-patch-1 branch June 25, 2018 23:46
@tpodolak tpodolak mentioned this pull request Jul 18, 2018
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