Skip to content

Conversation

@TorbenRahbekKoch
Copy link
Contributor

In reaction to issue #299 I have added a hash based check, which then only transpiles the .jsx file if it has actually changed. This shortens build times considerably.

@Daniel15
Copy link
Member

Daniel15 commented Sep 3, 2016

Please use tabs rather than spaces. You can use EditorConfig to automatically use the correct indentation configuration in Visual Studio.

Also, please add a unit test testing this functionality :)

/// <param name="inputFileContents">The contents of the input file.</param>
/// <param name="outputPath">The output path of the (possibly previously) generated file.</param>
/// <returns>Returns true if the file should be transpiled, false otherwise.</returns>
public virtual bool MustTranspileFile(string inputFileContents, string outputPath)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would prefer calling this ValidateCache and making it return true if the cache is valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ValidateCache is rather vague and unspecific - CacheIsValid would be a better name, since it better hints at what state the cache is actually in, when true/false is returned.

And I like flipping the true/false values, since it would enable me to turn around the if in your next comment ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, CacheIsValid or CheckCacheValidity would be fine.

Also please make it private, and it does not need to be virtual.

@TorbenRahbekKoch
Copy link
Contributor Author

EditorConfig seems like a nice tool. Maybe you should add more .editorconfig files to the relevant parts of the solution ;)

Sorry about the missing unit-test. That was a brain fart on my part.

All in all good suggestions - I will get back to you with some updates ;)

@Daniel15
Copy link
Member

Daniel15 commented Sep 4, 2016

Ahh, my mistake, I didn't realise this project was missing an editorconfig file! This is the config I normally use: https://github.com/Daniel15/RouteJs/blob/master/.editorconfig. I must have forgotten to copy it across to this repository 😄

@Daniel15
Copy link
Member

Daniel15 commented Sep 4, 2016

(although I've used tabs for a long time, I'm tempted to switch to spaces since at work we always use two spaces for indentation)

@TorbenRahbekKoch
Copy link
Contributor Author

Thanks for the config ;) I think that, in general, using spaces seems to cause the least hazzle ;)

@TorbenRahbekKoch
Copy link
Contributor Author

I think I'm done now ;)

@Daniel15 Daniel15 merged commit 2ca1cd0 into reactjs:master Sep 4, 2016
@Daniel15
Copy link
Member

Daniel15 commented Sep 4, 2016

Thank you! :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants