-
Notifications
You must be signed in to change notification settings - Fork 30
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
added SaveCodeToFileAsync method #101
base: master
Are you sure you want to change the base?
Conversation
…Filesystem methods
I also added checks to create the directory path if it does not exist. See here for discussion: #100 (comment) |
src/Testura.Code/Saver/CodeSaver.cs
Outdated
|
||
private void EnsurePathExists(string filePath) | ||
{ | ||
var fi = new FileInfo(filePath); |
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.
Minor thing but should maybe use the full name for FileInfo variables? So var fileInfo = new FileInfo(path)
.
I just think it's easier to read and understand when we use clear names for a variable (which may sound a bit like hypocrisy as I named CompilationUnitSyntax cu
...). But it's more about taste and if you prefer fi we can stick with that.
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.
Sounds good! I am always for more 'explicit' style of programming, even if its more verbose - its easier to understand.
Haha, yea we all get lazy at times, thats why I use fi
, its just laziness haha. I'll make the change now
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.
Done! I changed a few just to make them more explicit. I'd love to do this everywhere in the code eventually.
I'd also love to add more constructors and such so we can avoid using null
as a parameter Such as this example:
// I'd love to remove the null, so the developer can just skip the 'WithBody' part and get the same result
var builder = new MethodBuilder("MyMethod").WithBody(null).Build()
It will create this:
void MyMethod();
I won't do that in this PR - but I just wanted to mention it as its related to the topic of being more explicit
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.
Sounds like a good idea to remove withbody requirment. Approved the PR as well.
Yeah it seems like a good thing to add as well. |
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.
Looks good!
here is the PR for #100
My editor formatted the
.editorconfig
files, I don't think its a problem, but let me know if you'd like me to make any changes.Thanks!