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.

Sign up for GitHub

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

Jump to bottom

Refactor document types #90

Merged
merged 14 commits into from
Aug 12, 2020
Merged

Refactor document types #90

merged 14 commits into from
Aug 12, 2020

Conversation

Ndpnt
Copy link
Member

@Ndpnt Ndpnt commented Aug 10, 2020

No description provided.

@Ndpnt Ndpnt requested a review from MattiSG August 10, 2020 14:33
MattiSG
MattiSG requested changes Aug 10, 2020
View reviewed changes
Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll improve the copywriting on the CONTRIBUTING 🙂

Congrats for the tryptic descriptions, I personally find them very clear and useful!

I love how simpler this all becomes after unifying file name, ID and name! 😃

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
package.json Outdated
"start:scheduler": "node schedule.js",
"start:scheduler:prod": "NODE_ENV=production node schedule.js",
"setup": "node scripts/setup.js",
"start": "node --experimental-json-modules index.js",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer that we use createRequire when needed, as recommended.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see where it's recommended.
They recommend to use createRequire instead of --experimental-modules but here I use --experimental-json-modules and about this one, they said:

A separate flag --experimental-json-modules enables experimental support for import of JSON files. There is work in progress for standardizing this feature with browsers, and Node.js hopes to align our support with the eventual standard.

Do you still prefer recreating the require function?

Copy link
Member Author

@Ndpnt Ndpnt Aug 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover, --experimental-json-modules is only required for the npm run validate command as we remove the TYPES import from all others files. (I just have to remove this instruction from other commands)
So the experimental feature won't affect the production scripts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see where it's recommended.

You're right, I'm sorry, the second paragraph you mentioned does add details to the initial statement.

Do you still prefer recreating the require function?

Yes, considering the stage of advancement of the standardisation for JSON modules (see whatwg/html#4315), I would feel much safer relying on a stable API rather than a flag whose behavior is less specified.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay :)

src/types.json Outdated Show resolved Hide resolved
src/types.json Outdated
"object": "API and Content usage"
}
},
"Developer Policy": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference with Developer Agreement?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find one. But Twitter define both.
If you can explain the difference:
https://developer.twitter.com/fr/developer-terms/agreement
https://developer.twitter.com/fr/developer-terms/policy

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.
I feel that the “agreement” is a full, legally binding contract, whereas the “policy” describes what Twitter expects and how it can be expected to behave.

I find this separation very interesting, but a bit uncommon as well.

I see two options, in order of preference:

  1. Rewrite the object of the Developer Policy as “expected behavior of and towards third-party services”.
  2. Stop tracking the Developer Policy for Twitter as of now, since we'll need to cover the specific case of Twitter and its multitude of documents in some special way anyway.

I'll let you decide.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we keep both, which one will you use for theses documents?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, it's a mess 🙃

In favor of option 2 then.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, if we keep one, which one do you prefer, "Developer Terms", "Developer Policy", "Developer Agreement"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“Policy” seems more common, isn't it? Otherwise I'd say “Developer Terms”, by similarity with “Terms of Service“.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I also found that "Policy" seems more common but as you explained for Twitter where “agreement” is a full, legally binding contract, whereas the “policy” describes what Twitter expects and how it can be expected to behave, "agreement" seems to be more in line with what we want.
Another point, if we keep "Policy", for Twitter the url is not the one which target Twitter Policy but this one "https://developer.twitter.com/developer-terms/agreement" which target the agreement.
So, even if "Developer Terms" seems less common, I think it avoids confusion while still being very understandable (and similar with “Terms of Service“ as you said), so I vote for this one.

@Ndpnt
Copy link
Member Author

Ndpnt commented Aug 10, 2020

Congrats for the tryptic descriptions, I personally find them very clear and useful!

Great :)

I love how simpler this all becomes after unifying file name, ID and name! 😃

Totally! 😃

Ndpnt
Ndpnt commented Aug 10, 2020
View reviewed changes
CONTRIBUTING.md Outdated Show resolved Hide resolved
This was referenced Aug 10, 2020
Closed
Closed
@Ndpnt
Copy link
Member Author

Ndpnt commented Aug 10, 2020

@michielbdejong What do you think about this document types refactor?

@Ndpnt Ndpnt force-pushed the refactor-document-types branch from 155b9a5 to a4ba8ad Compare August 11, 2020 06:40
MattiSG
MattiSG approved these changes Aug 11, 2020
View reviewed changes
Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@Ndpnt Ndpnt force-pushed the refactor-document-types branch from a4ba8ad to 4ef1ace Compare August 11, 2020 08:47
@Ndpnt Ndpnt force-pushed the refactor-document-types branch from 4ef1ace to 93a0502 Compare August 12, 2020 08:26
MattiSG added 3 commits August 12, 2020 11:06
@MattiSG
Rename document types
f6aa378 
Controller-Controller Data Protection Terms -> Data Controller Agreement
Data Processing Terms -> Data Processor Terms
Copyright Policy -> Copyright Claims Policy
@MattiSG
Fix GDPR-related document types tryptics
12f07ff
@MattiSG
Improve document types descriptions
c780786 
Move Parent Organization Terms below more common types
@MattiSG MattiSG force-pushed the refactor-document-types branch from 9ee8264 to c780786 Compare August 12, 2020 09:06
MattiSG
MattiSG approved these changes Aug 12, 2020
View reviewed changes
Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rewrote GDPR-related tryptics. LGTM now!

I did not execute the code.

:shipit:

@Ndpnt Ndpnt merged commit 52de1e1 into master Aug 12, 2020
@Ndpnt Ndpnt deleted the refactor-document-types branch August 12, 2020 09:23
@michielbdejong
Copy link
Contributor

Great! So far we haven't used document type strictly in tosback.org, nor in tosdr.org, so I'll probably keep a crawler running somewhere that also crawls "other" documents that don't fit in this more structured list of documents by type by service. But I'll try to assign one of the new document types to each one! Maybe that can also be part of the user interface of edit.tosdr.org, so volunteers can do this work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MattiSG MattiSG MattiSG approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Ndpnt @michielbdejong @MattiSG