Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[TS migration] Migrate 'Device' lib to TypeScript #27709
[TS migration] Migrate 'Device' lib to TypeScript #27709
Changes from 5 commits
2276094
8786f68
06a907c
ce892be
660a8a3
a3cfbf6
c47531d
02b524c
4ab974a
e87a349
90773e2
ac5668c
686c0ad
ae6f0aa
1008c6c
9162088
7928fc1
a468073
ae92c8d
ee562a9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I am working on the PR, but when testing, I can see the command still passes the snake case params, which are coming from the expensify-common here. Is this intentional? I am a bit confused about this?
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.
Yes, you are right @mountiny . I think we should leave these ones as they are, to avoid making additional changes to
expensify-common
. I checked the lib and there are a lot of exported properties there that don't follow naming conventions here.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.
Alright, well then there is not much to do here. I am going to let the backend PR go out for review and update here when ready. I think the
app_version
is the only oneThere 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.
Ah this is only for web so in native it will be sent differently
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.
I disagree, I think we should update them all to match our style guide. Is there a reason that can't be done? I understand it's more work, but we should follow our style guide
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.
But netherless, in case we would do such changes we shouldn't block this PR because of it, as I understand
expensify-common
is used in several other projects, right?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.
I'm pretty sure we use our public style guide in all our repos, at least our internal ones. But maybe our App style guide is different.
If there's no updates needed in this code base after updating expensify-common, then we shouldn't block this PR on that update. Otherwise, I think we should update expensify-common first
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.
I will update it tomorrow
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.
Hmm I don't have access to this repo, but this naming conventions is an ESLint rule that we applied only to TS files in E/App.
@mountiny @cead22 We would have to change the params that are using snake case. So, do you want to proceed with expensify-common PR first and putting this on HOLD?
cc @kubabutkiewicz Since I will be OOO for the next two weeks.
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.
I will make the expensify-common change
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.