refactor: move some files around, fix loose ends and fix broken redirect #970

fabiosantoscode · 2020-02-04T13:14:07Z

Fixing some loose ends with #943, addressing @jorgeorpinel and @shcheklein's feedback. Also fixed the troubleshooting redirect URL which was missing the /doc prefix.

❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.

🐛 Please make sure to mention Fix #issue (if applicable) in the description of the PR. This causes GitHub to close it automatically when the PR is merged.

Please chose to
allow us
to edit your branch when creating the PR.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

fabiosantoscode · 2020-02-04T13:50:48Z

link-check-git-diff is failing because I've removed the broken redirects, and it thinks those were links that worked.

shcheklein · 2020-02-04T15:26:24Z

redirects-list.json

  "^https://(?:www\\.)dvc\\.org/(.+)?       https://dvc.org/$1",
  "^https://man\\.dvc\\.org/(.+)?           https://dvc.org/doc/command-reference/$1 303",
-  "^https://error\\.dvc\\.org/(.+)?         https://dvc.org/user-guide/troubleshooting#$1 303",
+  "^https://error\\.dvc\\.org/(.+)?         https://dvc.org/doc/user-guide/troubleshooting#$1 303",


good catch!

Oops. Maybe we should try to thoroughly test redirects manually (sanity check) even if we have tests because manually you can come up with ways to try to break the logic 😋 This is because publishing incorrect redirects (especially 301) causes almost permanent "damage" to new users browser cache. Just a thought

Needless to say I'm available to do this but just need to prioritize as I've been struggling with switching from task to task recently.

shcheklein · 2020-02-04T15:28:46Z

looks good!

@fabiosantoscode please add those failed links to the exclusion list please.

shcheklein

check the comment please, just a minor thingy

fabiosantoscode · 2020-02-04T17:28:07Z

Cool! I've also used sed to remove the hash part, instead of adding three links which differ only by hash but are the same document.

shcheklein · 2020-02-04T17:34:03Z

scripts/exclude-links.txt

 https://github.com/iterative/dvc.org/blob/master/public$
 https://github.com/iterative/dvc/releases/download/$
 https://github.com/myaccount/myproject.git
+https://dvc.org/user-guide/troubleshooting


so, where does this link come from? I don't quite understand

removing this for now and merging the PR

src/utils/redirects.js

jorgeorpinel

I know this is merged but for the record I approve haha. I do have some JS code suggestions thought, may open a separate PR or include in my current regular updates one (#966)

shcheklein · 2020-02-04T18:35:18Z

@jorgeorpinel because it's website global config. It's a regular thing to have some redirects file in the root. e.g. Netlify, as far as I remember is just one of the examples.

shcheklein · 2020-02-04T18:37:28Z

I know this is merged but for the record I approve haha. I do have some JS code suggestions thought, may open a separate PR or include in my current regular updates one (#966)

please, if it's possible let's not do this unless they are just stylistic changes (like reoder ifs, or something) that are not directly related to this PR. Otherwise, please leave some comments here - we'll try to address this while we still have context. cc @jorgeorpinel

jorgeorpinel · 2020-02-04T18:46:22Z

let's not do this unless they are just stylistic changes

They are small refactors and language/stylistic changes indeed. They don't change the behavior. I can open a sample draft PR so you guys see them... ⏳

Also, I do still find the space separation in the JSON file strange. Why not instead of:

[
  "^https://(?:www\\.)dvc\\.org/(.+)?       https://dvc.org/$1",
  ...

do something like

[
  ["^https://(?:www\\.)dvc\\.org/(.+)?", "https://dvc.org/$1", 308],
  ...

?

jorgeorpinel · 2020-02-04T18:49:34Z

I thin think is Ivan's logic on the redirects JSON file:

From #943 (comment)
we had an iteration like this and I didn't like it (not sure about @fabiosantoscode :) ) - it's 1000 lines vs 10. It's way easier to read line by line. It's very regular format for redirects files - one rule a line. There should be very strong reasons to complicate this.

So why use JSON at all? Probably a regular redirect file is just plan text? Even simpler. Up to you guys...

p.s. re it's 1000 lines vs 10: Not sure how though. See my suggestion in #970 (comment) above.

I won't touch this file in my small refactoring suggestion though. Let's see what Fabio prefers 🙂

shcheklein · 2020-02-04T18:53:22Z

They are small refactors and language/stylistic changes indeed. They don't change the behavior. I can open a sample draft PR so you guys see them... ⏳

if they are not related to this PR at all - include them to the regular updates, up to you!

So why use JSON at all?

simpler to deal with in JS

do something like array of fields

one line array instead of a string might work fine as well, don't have a strong opinion here

shcheklein · 2020-02-04T18:54:27Z

one line array instead of a string might work fine as well, don't have a strong opinion here

would probably still personally prefer a simple string, just don't see a reason to write all these additional symbols in this case :)

as mentioned in #970 (review)

jorgeorpinel · 2020-02-04T19:11:16Z

if they are not related to this PR at all - include them to the regular updates, up to you!

They're related, see https://github.com/iterative/dvc.org/pull/973/files

Thanks

fabiosantoscode · 2020-02-04T19:38:34Z

@jorgeorpinel I've added the spaces because it was pretty hard to read the big-picture of the file without them. I'm happy to remove if you feel strongly against them.

I also think arrays would be fine. Maybe the space alignment would be more natural in that situation, or we could do something like this to make it more readable, even if it's taller:

[
  [
    "/from",
    "/to",
    308
  ],
[...]

jorgeorpinel · 2020-02-04T19:43:01Z

In my case my IDE wraps at 80 chars so taller is easier to read in cases like this. ESLint would probably reformat it that way anyway. Again, it's not a strong opinion but I do think it's a better format and less error prone for later edits (how many spaces should I add? space typos, etc.)

shcheklein · 2020-02-04T20:08:03Z

still prefer compact vs very tall - it's just hard to see the whole picture when it's tall, means hard to catch errors. In this case you just see clearly one block that looks exactly like:

string     string
string     string
string     string

it's super easy to read and see all at once. Especially if you start grouping them by the first string.

jorgeorpinel · 2020-02-04T22:40:21Z

This is how it looks for me 😋

13" laptop screen, and clearly I like large fonts 😬

jorgeorpinel · 2020-02-04T22:41:37Z

BTW lines 4 and 5 don't have the same space alignment on the 2nd column:

  "^https://error\\.dvc\\.org/(.+)?         https://dvc.org/doc/user-guide/troubleshooting#$1 303",
  "^https://(code|data|remote)\\.dvc\\.org/(.+) https://s3-us-east-2.amazonaws.com/dvc-public/$1/$2 303",

shcheklein · 2020-02-04T22:43:50Z

I'm not sure 100% but my intuition is that you don't set auto-wrapping for code related stuff - it's extremely dangerous.

shcheklein · 2020-02-04T22:45:51Z

also, you at least have a chance to make it wider for a moment, in case it's long - no one has a chance whatsoever to see it all at once, group it, etc

jorgeorpinel · 2020-02-04T23:41:54Z

editor line wrapping = extremely dangerous? 😱 well I need some adrenaline in life

Didn't quite get that last comment about "see it all at once, group it, etc" but OK, those are all my arguments and still my position isn't very strong about this, plus I'm not convincing anyone, so let's just move on 🙂

* server: reformat/refactor code and comments around new redirects as mentioned in #970 (review) * links: addresses #973 PR feedback per #973 (review) through #973 (review)

as mentioned in #970 (review)

refactor: move some files around, fix loose ends and fix broken redirect

ee475e0

fabiosantoscode requested a review from jorgeorpinel February 4, 2020 13:18

shcheklein reviewed Feb 4, 2020

View reviewed changes

shcheklein suggested changes Feb 4, 2020

View reviewed changes

exclude links from link-check

b91eed7

shcheklein reviewed Feb 4, 2020

View reviewed changes

Update exclude-links.txt

905d51f

shcheklein approved these changes Feb 4, 2020

View reviewed changes

shcheklein merged commit e71c7b3 into treeverse:master Feb 4, 2020

jorgeorpinel reviewed Feb 4, 2020

View reviewed changes

src/utils/redirects.js Show resolved Hide resolved

jorgeorpinel reviewed Feb 4, 2020

View reviewed changes

jorgeorpinel added a commit that referenced this pull request Feb 4, 2020

server: reformat/refactor code and comments around new redirects

ca28a4d

as mentioned in #970 (review)

jorgeorpinel mentioned this pull request Feb 4, 2020

server: reformat/refactor around redirects #973

Merged

weekly-digest bot mentioned this pull request Feb 9, 2020

Weekly Digest (2 February, 2020 - 9 February, 2020) #984

Closed

jorgeorpinel added a commit that referenced this pull request Feb 20, 2020

server: reformat/refactor code and comments around new redirects

be81e93

as mentioned in #970 (review)

weekly-digest bot mentioned this pull request Feb 23, 2020

Weekly Digest (16 February, 2020 - 23 February, 2020) #1015

Closed

refactor: move some files around, fix loose ends and fix broken redirect #970

refactor: move some files around, fix loose ends and fix broken redirect #970

Uh oh!

Conversation

fabiosantoscode commented Feb 4, 2020

Uh oh!

fabiosantoscode commented Feb 4, 2020

Uh oh!

shcheklein Feb 4, 2020

Choose a reason for hiding this comment

Uh oh!

jorgeorpinel Feb 4, 2020

Choose a reason for hiding this comment

Uh oh!

jorgeorpinel Feb 4, 2020

Choose a reason for hiding this comment

Uh oh!

shcheklein commented Feb 4, 2020

Uh oh!

shcheklein left a comment

Choose a reason for hiding this comment

Uh oh!

fabiosantoscode commented Feb 4, 2020

Uh oh!

shcheklein Feb 4, 2020

Choose a reason for hiding this comment

Uh oh!

shcheklein Feb 4, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jorgeorpinel left a comment

Choose a reason for hiding this comment

Uh oh!

shcheklein commented Feb 4, 2020

Uh oh!

shcheklein commented Feb 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jorgeorpinel commented Feb 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jorgeorpinel commented Feb 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shcheklein commented Feb 4, 2020

Uh oh!

shcheklein commented Feb 4, 2020

Uh oh!

jorgeorpinel commented Feb 4, 2020

Uh oh!

fabiosantoscode commented Feb 4, 2020

Uh oh!

jorgeorpinel commented Feb 4, 2020

Uh oh!

shcheklein commented Feb 4, 2020

Uh oh!

jorgeorpinel commented Feb 4, 2020

Uh oh!

jorgeorpinel commented Feb 4, 2020

Uh oh!

shcheklein commented Feb 4, 2020

Uh oh!

shcheklein commented Feb 4, 2020

Uh oh!

jorgeorpinel commented Feb 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shcheklein commented Feb 4, 2020 •

edited

Loading

jorgeorpinel commented Feb 4, 2020 •

edited

Loading

jorgeorpinel commented Feb 4, 2020 •

edited

Loading