Add check for ungenerated code #679

DanielMSchmidt · 2018-04-20T07:13:46Z

We had at times updates of the source for code generation not being reflected in the adapters as the code was not generated again. Let's add a check for this here 👍

rotemmiz · 2018-04-20T17:39:54Z

generation/check-for-git-changes.sh

+set -e
+
+if [[ $(git diff) ]]; then
+    echo "Git shows that there are changes."


Please be more explicit in these logs.

What happened ?

Why did it happen ?

What should the user do now in order to resolve it ?

Added meaningful error message

DanielMSchmidt · 2018-04-25T19:07:09Z

@rotemmiz added error message, could you review it again?

rotemmiz · 2018-04-25T21:16:50Z

detox/src/ios/earlgreyapi/GREYCondition.js

@@ -19,18 +17,27 @@ GREYCondition::waitWithTimeout:pollInterval:
 @param seconds Amount of time to wait for the condition to be met, in seconds.

 @return @c YES if the condition was met before the timeout, @c NO otherwise.
-*/static waitWithTimeout(element, seconds) {
-    if (typeof seconds !== "number") throw new Error("seconds should be a number, but got " + (seconds + (" (" + (typeof seconds + ")"))));
+*/ static waitWithTimeout(


Is this the crazy working of prettier? This is bad formatting IMO. It's much harder to actually read the function signature like this.

rotemmiz · 2018-04-25T21:19:57Z

generation/check-for-git-changes.sh

+npm run build
+
+if [[ $TRAVIS && $(git diff) ]]; then
+    printf "\n\n"


rotemmiz · 2018-04-25T21:20:38Z

generation/index.js

@@ -1,4 +1,7 @@
 #!/usr/bin/env node
+const prettier = require("prettier");


Yep, here it is...

DanielMSchmidt · 2018-04-25T22:03:03Z

@rotemmiz I understood that we wanted to use prettier in our project, so I thought adding it to the generated code too makes sense. I think we are doing a suboptimal job enforcing it, I would offer to help with that. The mockserver I am currently writing enforces it with the help of jest, which I like very much, you can see the config here.

I found this quote on a podcast I listen to (freely quoted as I don't remember when they said it):

Prettier reformats your code to look worse, but at the same time it makes your co-workers code much more readable.

This resonated a lot with me, I think everyone has an opinion on how code should be looking like for whatever (totally) reasons. prettier frees us from having to have conversations about how it should look like, it just always looks the same, freeing up brain cycles for more important tasks 😇

rotemmiz · 2018-04-25T22:09:08Z

You're right but I still really don't like these formatting choices. It makes the code longer (in lines), and very weirdly formatted.
I would want to explore it there are different "flavors" tho this formatting before running it on the whole project. I do like ESLint's defaults much better.
Sorry for delaying this for too long, but these formatting chocices made by prettier really hurt my eyes.

DanielMSchmidt · 2018-04-28T06:57:57Z

Prettier has a nice playground and at the bottom left corner there is a "Show Options" button where you can play with the options until you like the output ;)

rotemmiz · 2018-05-03T09:21:53Z

detox/src/ios/earlgreyapi/GREYInteraction.js

-*/static assertWithMatcher(element, matcher) {
-    if (typeof matcher !== "object" || matcher.type !== "Invocation" || typeof matcher.value !== "object" || typeof matcher.value.target !== "object" || matcher.value.target.value !== "GREYMatchers") {
-      throw new Error('matcher should be a GREYMatcher, but got ' + JSON.stringify(matcher));
+*/ static assertWithMatcher(


Turns out this is not "prettier being prettier".
Prettier doing a really bad job here because of the */ is at the same line as the function deceleration.
If you make sure docs and function declarations are in different lines it will work quite good.

Here's a config I feel ok with.

Hey @DanielMSchmidt , can you please take a look at this?

I'm on it, thank you for pinging 👍

DanielMSchmidt · 2018-05-08T17:51:00Z

@rotemmiz I think we are there now, please take another look 🙏

Might make sense to go over it commit by commit as the changeset is quite big due to prettier being unleashed on the generated code

Not up to date anymore

rotemmiz · 2018-05-08T18:27:47Z

I hate this argument as well, but please use spaces. Tabs make it much harder to read on GitHub, and we spend a lot of time reviewing here

DanielMSchmidt · 2018-05-08T19:20:29Z

@rotemmiz done 👍 Did I copy the rest of your prettier session correctly?

rotemmiz · 2018-05-08T19:33:02Z

detox/src/ios/earlgreyapi/GREYActions.js

-    if (typeof point !== "object") throw new Error("point should be a object, but got " + (point + (" (" + (typeof point + ")"))));
-    if (typeof point.x !== "number") throw new Error("point.x should be a number, but got " + (point.x + (" (" + (typeof point.x + ")"))));
-    if (typeof point.y !== "number") throw new Error("point.y should be a number, but got " + (point.y + (" (" + (typeof point.y + ")"))));
+*/ static actionForMultipleTapsWithCountAtPoint(


still here!
separate */ static actionForMultipleTapsWithCountAtPoint( to

*/ static actionForMultipleTapsWithCountAtPoint(

Can't do much about it, this would be something we would need to do in prettier itself, providing an upstream PR. Do you really want me to spend the time there? Would be fine with that, just wanted to clarify.

It's not related to prettier, it's the code generator

DanielMSchmidt · 2018-05-09T06:56:52Z

Unfortunately, I can't do much about it. I add a leadingComments entry for the comment in babel, which means "insert the comment before this node", leading to the output you see. So my options are either

Go into babel and "fix" the leadingComments behaviour
Go into prettier and enable it to prettify this situation
Go with a hack solution (String replacement of the final output)
Let it be like this

Which route would you like to take? 😇

rotemmiz · 2018-05-09T07:09:13Z

4

DanielMSchmidt · 2018-05-09T07:42:58Z

Okay :)

You settled for option 4 - let it be like this, so I dismiss your review

DanielMSchmidt · 2018-05-12T10:01:33Z

@rotemmiz Could you give a final review here? 😇

rotemmiz · 2018-05-12T11:19:29Z

detox/src/ios/earlgreyapi/GREYInteraction.js

@@ -126,21 +167,25 @@ to be selected.

 @return An interaction (assertion or an action) to be performed on the element at the
 specified index in the list of matched elements.
-*/static atIndex(element, index) {
-    if (typeof index !== "number") throw new Error("index should be a number, but got " + (index + (" (" + (typeof index + ")"))));
+*/ static atIndex(


Still broken here :/

rotemmiz · 2018-05-12T11:22:06Z

The current state where the end of multiline comment is at the beginning of the function declaration fucks up prettier.
Please drop prettier untill there's a solution for these line breaks

DanielMSchmidt · 2018-05-12T13:05:30Z

@rotemmiz I removed prettier from the generated code (only applied the new style to the code generation). I opened an issue at prettier/prettier#4465 let's see if we can get any help there. I might investigate if there is a quick fix within prettier that doesn't take too long.

rotemmiz · 2018-05-12T13:44:40Z

.prettierrc

@@ -0,0 +1,8 @@
+{
+  "useTabs": true,


useTabs:false!!

Sorry, forgot to squash one of the commits I dropped

In this step we also changed the config to standards that match our code styles better.

This file gets rewritten with an npm install, so it leads to unnecessary changes

DanielMSchmidt · 2018-05-12T15:20:44Z

@rotemmiz Sorry for the typo :/

rotemmiz · 2018-05-12T17:26:31Z

@DanielMSchmidt can you please take a look, seems like the build is broken due to this change.
https://travis-ci.org/wix/detox/jobs/378118106#L657

DanielMSchmidt · 2018-05-12T17:37:26Z

Yay, the check works. But damn, need to fix them 😂 will do that asap and ping you :)

DanielMSchmidt requested review from LeoNatan and rotemmiz as code owners April 20, 2018 07:13

rotemmiz previously requested changes Apr 20, 2018

View reviewed changes

DanielMSchmidt force-pushed the danielmschmidt/check-if-generation-needed branch 4 times, most recently from 7ddc373 to a09fa44 Compare April 25, 2018 19:05

DanielMSchmidt force-pushed the danielmschmidt/check-if-generation-needed branch from a09fa44 to c2e51c4 Compare April 25, 2018 19:32

rotemmiz reviewed Apr 25, 2018

View reviewed changes

DanielMSchmidt self-assigned this Apr 25, 2018

rotemmiz previously requested changes May 3, 2018

View reviewed changes

DanielMSchmidt force-pushed the danielmschmidt/check-if-generation-needed branch 2 times, most recently from 775281b to 5f435e9 Compare May 8, 2018 17:49

rotemmiz previously requested changes May 8, 2018

View reviewed changes

rotemmiz reviewed May 12, 2018

View reviewed changes

add check for ungenerated code

2a0dbd6

DanielMSchmidt force-pushed the danielmschmidt/check-if-generation-needed branch from 51b8214 to 7ef345f Compare May 12, 2018 12:55

DanielMSchmidt closed this May 12, 2018

DanielMSchmidt reopened this May 12, 2018

rotemmiz reviewed May 12, 2018

View reviewed changes

DanielMSchmidt and others added 3 commits May 12, 2018 17:20

consolidate prettierrc for subprojects

5b756f6

In this step we also changed the config to standards that match our code styles better.

ignore package.json with prettier

bd10a75

This file gets rewritten with an npm install, so it leads to unnecessary changes

apply prettier styles to code generation

a27e2f2

DanielMSchmidt force-pushed the danielmschmidt/check-if-generation-needed branch from 7ef345f to a27e2f2 Compare May 12, 2018 15:20

regenerated adapter code

1aee0d0

rotemmiz approved these changes May 17, 2018

View reviewed changes

rotemmiz merged commit c6b1b67 into master May 17, 2018

DanielMSchmidt deleted the danielmschmidt/check-if-generation-needed branch May 17, 2018 11:24

wix locked and limited conversation to collaborators Jul 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add check for ungenerated code #679

Add check for ungenerated code #679

DanielMSchmidt commented Apr 20, 2018

rotemmiz Apr 20, 2018

DanielMSchmidt commented Apr 25, 2018

rotemmiz Apr 25, 2018

rotemmiz Apr 25, 2018

rotemmiz Apr 25, 2018

DanielMSchmidt commented Apr 25, 2018

rotemmiz commented Apr 25, 2018

DanielMSchmidt commented Apr 28, 2018

rotemmiz May 3, 2018

rotemmiz May 8, 2018

DanielMSchmidt May 8, 2018

DanielMSchmidt commented May 8, 2018

rotemmiz commented May 8, 2018

DanielMSchmidt commented May 8, 2018

rotemmiz May 8, 2018

DanielMSchmidt May 8, 2018

rotemmiz May 9, 2018

DanielMSchmidt commented May 9, 2018 •

edited

Loading

rotemmiz commented May 9, 2018

DanielMSchmidt commented May 9, 2018

DanielMSchmidt commented May 12, 2018

rotemmiz May 12, 2018

rotemmiz commented May 12, 2018

DanielMSchmidt commented May 12, 2018

rotemmiz May 12, 2018

DanielMSchmidt May 12, 2018

DanielMSchmidt commented May 12, 2018

rotemmiz commented May 12, 2018

DanielMSchmidt commented May 12, 2018

		@@ -1,4 +1,7 @@
		#!/usr/bin/env node
		const prettier = require("prettier");

Add check for ungenerated code #679

Add check for ungenerated code #679

Conversation

DanielMSchmidt commented Apr 20, 2018

Choose a reason for hiding this comment

DanielMSchmidt commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielMSchmidt commented Apr 25, 2018

rotemmiz commented Apr 25, 2018

DanielMSchmidt commented Apr 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielMSchmidt commented May 8, 2018

rotemmiz commented May 8, 2018

DanielMSchmidt commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielMSchmidt commented May 9, 2018 • edited Loading

rotemmiz commented May 9, 2018

DanielMSchmidt commented May 9, 2018

DanielMSchmidt commented May 12, 2018

Choose a reason for hiding this comment

rotemmiz commented May 12, 2018

DanielMSchmidt commented May 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielMSchmidt commented May 12, 2018

rotemmiz commented May 12, 2018

DanielMSchmidt commented May 12, 2018

DanielMSchmidt commented May 9, 2018 •

edited

Loading