Setting up mobile app #19
Conversation
|
Holy shit 50 files for initialization lol, I'll get it reviewed tomorrow. |
|
A lot of the files are auto generated when a react native project is initialized. Files that are probably of interest:
|
|
Not a review, but I just want to say I'm really impressed. This manages to preserve code coverage, unit tests and the babel compilation. I'm still fighting hard with all these things on the Vue template |
tsconfig.json
Outdated
| /* Additional Checks */ | ||
| // "noUnusedLocals": true, /* Report errors on unused locals. */ | ||
| // "noUnusedParameters": true, /* Report errors on unused parameters. */ | ||
| // "noImplicitReturns": true, /* Report error when not all code paths in function return a value. */ |
There was a problem hiding this comment.
These 3 returns seem pretty useful to me, what do you think of turning them on?
There was a problem hiding this comment.
I think that this can be something configured later on when we configure linting (ie. figuring out if we should not allow code to be compiled properly if these rules are broken vs. failing lint tests when these rules are broken).
Also, this was also auto-generated after I followed Microsoft's guide for integrating typescript with react native: https://github.com/Microsoft/TypeScript-React-Native-Starter
| @@ -0,0 +1,56 @@ | |||
| { | |||
| "name": "trader", | |||
| "version": "0.0.1", | |||
There was a problem hiding this comment.
In my experience with deployment, we need to include a bigger builder number in this version number that auto-increments. Doesn't need to be now, but it'll become important when we start passing it around.
There was a problem hiding this comment.
I think this will be out of the scope of this issue so that this doesn't block the other tasks anymore. I raised an issue for auto-incrementing our versions in general: agriculture-capstone/systems#2
| @@ -0,0 +1,56 @@ | |||
| { | |||
| "name": "trader", | |||
There was a problem hiding this comment.
This feels too generally named, not a issue specific with this PR, but do you think this project needs a better name?
There was a problem hiding this comment.
I don't think this matters too much here because this name here is used for making an npm package but yeah we can probably find a better name for the app (might take some work to refactor this though haha)
There was a problem hiding this comment.
renamed the app to agritrader
| "branches": 100, | ||
| "functions": 100, | ||
| "lines": 100, | ||
| "statements": 100 |
There was a problem hiding this comment.
I don't feel like we need to mandate a certain amount of test coverage, what's your experiences with this field?
There was a problem hiding this comment.
I think it would help to maintain certain thresholds for test coverage (obviously not 100%) but it would be good later on (especially when it is being maintained by other people). From my experience, when a lot of refactoring needed to be done, it was very scary making changes to a project with little unit tests because you won't know when you are unintentionally breaking something. It'll just be here to help make sure the quality of our software is good :)
I think we can also integrate code coverage with jenkins if that's something of interest!
package.json
Outdated
| { | ||
| "name": "trader", | ||
| "version": "0.0.1", | ||
| "private": true, |
There was a problem hiding this comment.
We might want to include
{ "license": "UNLICENSED" }
cover our ass. I think changing that will be important in the future.
beaesguerra
force-pushed
the
setting-up-app
branch
from
e043e9d to
c9ec71d
Compare
|
Closing and creating new pull request after renaming the project |
Sample screenshot of the application in its' basic state:
