Skip to content
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

Hash windows paths #1172

Merged
merged 14 commits into from Sep 27, 2019

Conversation

@nirinchev
Copy link
Member

commented Sep 24, 2019

Closes #793

@nirinchev nirinchev self-assigned this Sep 24, 2019
@nirinchev nirinchev requested a review from kraenhansen Sep 24, 2019
nirinchev added 2 commits Sep 24, 2019
@nirinchev

This comment has been minimized.

Copy link
Member Author

commented Sep 24, 2019

@kraenhansen I'm not typically one to debate coding style, but that is the exact opposite of pretty: 0ac6293#diff-9b5622305354798878e1836ed2d77d9dL90-R104. Do you think we can raise the line length limit to something more meaningful?

nirinchev added 6 commits Sep 24, 2019
Copy link
Member

left a comment

Overall great - is there any good reasons we want to do this only on Windows? It might make it simpler to reason about if we just made it the default on any platform?

// tslint:disable-next-line:no-console
console.log(
`Rewrote: '${user.identity} - ${getUrl(user, realmPath)}' to '${result}'`,
);

This comment has been minimized.

Copy link
@kraenhansen

kraenhansen Sep 25, 2019

Member

Do we really need this? I suggest removing it.

This comment has been minimized.

Copy link
@nirinchev

nirinchev Sep 25, 2019

Author Member

I think it's useful to know the name of the file that corresponds to a url. An alternative approach I was considering was to add a menu item in the browser to locate the file on disk, but that seemed too much for something so simple. I think it's mostly useful for debugging purposes (e.g. to see how large the client realm is after initial sync), so opening the debug tools and checking the console output seemed like the best tradeoff between preserving the functionality and not overengineering it.

This comment has been minimized.

Copy link
@kraenhansen

kraenhansen Sep 26, 2019

Member

In its current form Realm Studio is writing pretty much nothing to the console (except for Electron and Realm which writes directly to stdout), hence the no-console lint rule. (try searching the codebase).

In my experience it's easier to instrument the app with console.logs when you're debugging a particular issue (or attach a debugger?) then to graciously put them anywhere they might come in handy. You'll always miss that thing you needed anyway.

Perhaps we should add an easy way to get the size on disk of the client-side Realm instead?

This comment has been minimized.

Copy link
@nirinchev

nirinchev Sep 26, 2019

Author Member

I feel strongly there should be a way for us to reverse map a hashed path to a file on disk. Console.logging the path was the cheapest way to achieve that, considering Realm devs would be able to open the developer tools and inspect the console output. If you prefer to achieve that goal in a different way, feel free to contribute to the PR and expose menu options.

src/services/ros/realms.ts Show resolved Hide resolved
Jenkinsfile Show resolved Hide resolved
@kraenhansen

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

@kraenhansen I'm not typically one to debate coding style, but that is the exact opposite of pretty: 0ac6293#diff-9b5622305354798878e1836ed2d77d9dL90-R104. Do you think we can raise the line length limit to something more meaningful?

It's just left at the default (which looks to be 80, judging by https://prettier.io/docs/en/options.html#print-width and https://prettier.io/docs/en/rationale.html#print-width):

Prettier, on the other hand, strives to fit the most code into every line. With the print width set to 120, prettier may produce overly compact, or otherwise undesirable code.

In principle I don't see an issue with increasing the limit from 80. At the same time I can also see how you could easily break up the lines you reference into multiple lines using intermediary variables with meaningful names. There will always be some lines that hits right about the limit, whatever the limit might be.

What do you think a more meaningful limit would be?

@nirinchev

This comment has been minimized.

Copy link
Member Author

commented Sep 25, 2019

Re: why only on windows - I still feel the verbose paths are easier to read (if one has to) and considering we don't have limitations on other platforms, I don't see a good reason to rewrite the paths there.

Re: prettier - I think the width of the github code browser is good enough - I think that's around 120 characters. We could also try 100 or 120 and review the changes it makes and see if something becomes overly obnoxious.

nirinchev and others added 4 commits Sep 25, 2019
@kraenhansen kraenhansen force-pushed the ni/hash-windows-paths branch from b12f894 to 68e99a8 Sep 26, 2019
@kraenhansen kraenhansen merged commit bceddda into master Sep 27, 2019
1 check passed
1 check passed
continuous-integration/jenkins/pr-head This commit looks good
Details
@realm-probot

This comment has been minimized.

Copy link

commented Sep 27, 2019

Hey - looks like you forgot to add a T-* label - could you please add one (if you have access to add labels)?

@kraenhansen kraenhansen deleted the ni/hash-windows-paths branch Sep 27, 2019
@jtoselli

This comment has been minimized.

Copy link

commented Sep 30, 2019

Great enhancement :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.