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

Clean up error messages for container scenarios #221

Merged
merged 2 commits into from
Dec 16, 2019
Merged

Conversation

dakale
Copy link
Contributor

@dakale dakale commented Dec 12, 2019

Do either of you have opinions on changing some of these #if directives to plain c#? I think in general its more readable, and lets us remove a few specific cases of #pragma warning disable CS1998 where we are suppressing a warning only introduced because of the directives. Its also useful for when you are building on eg Windows, and may accidentally have some compile error in a !OS_WINDOWS block that only fails during CI. Similarly, the intellisense / finding references stuff seems to work better without the directives because it can find references to code made from inside those directive blocks, eg a reference to a function that is only called on linux and you are developing on windows or vice versa

On the other hand, I think it will generate a slightly larger assembly (looks like with these changes, the layout dir is ~1-2kb larger, which is probably negligible, and may also be due to other reasons), it will add an extra if check at runtime, but its just comparing static values and might get optimized out anyways (but could still be the source of a bug or error), and is a bit more verbose

Just wanted to get some early feedback before I finish the rest of this, totally fine sticking with the existing pattern of using directives for platform specific behavior

@TingluoHuang
Copy link
Member

How would you handle the following case?

#if OS_WINDOWS
    [ServiceLocator(Default = typeof(RSAEncryptedFileKeyManager))]
#else
    [ServiceLocator(Default = typeof(RSAFileKeyManager))]
#endif
    public interface IRSAKeyManager : IRunnerService
    { ... }

@dakale
Copy link
Contributor Author

dakale commented Dec 12, 2019

Not sure. Im not proposing changing it everywhere though, just reducing the usage. I might change it back as I work on this if I dont find more specific areas where it would be useful

@TingluoHuang
Copy link
Member

I am fine with it either way, go play with it and make a decision. 😄

@dakale dakale marked this pull request as ready for review December 16, 2019 17:54
@dakale
Copy link
Contributor Author

dakale commented Dec 16, 2019

Will merge by EOD but ready for explicit review, I dont think Ill add anything else here for now. Keeping the non-directive logic in places in functions that return Tasks, to avoid warnings about async logic only on certain platforms

@dakale dakale merged commit 1918906 into master Dec 16, 2019
@dakale dakale deleted the container-err-msg branch December 16, 2019 19:53
AdamOlech pushed a commit to antmicro/runner that referenced this pull request Jan 28, 2021
@yosmirnjr yosmirnjr mentioned this pull request Nov 30, 2022
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