Convert core/validation specs to non-IETF markdown #1425
Conversation
|
I've moved this PR to "Ready of Review". We may want to tweak some things as we go, but this should be a good enough place to start. Instructions on how the run the build and how to use the custom features are in the README changes. |
|
I can probably handle the GH Action setup. Let's get this through first. |
|
I forgot to mention. I think there's a lot of style/presentation choices we still need to make (or make consistently). I'd like to push that kind of thing to after the initial merge and address style/presentation issues individually. I'll address most of what's been brought up so far, but might ask that some things get figured out later. IMO, what's especially important to this PR is, do the plugins that I've configured or wrote fulfill our needs? Is there anything that you think should work differently? Is there anything that needs to be added? |
|
The latest push covers everything except the code title thing. |
gregsdennis
requested review from
karenetheridge,
Julian,
awwright and
Relequestual
|
The existing plugin for code titles has a significant limitation in that you can't have spaces in your titles, so I wrote a better one. I put up examples on the original poc demo page so you can see all the possilbe variations since there aren't examples of all in the spec. |
There was a problem hiding this comment.
Trusting the conversion here.
Left a few comments/suggestions unrelated to the content.
Overall, looks pretty darn good! Nice work!
Pinning dev tools and engines will avoid potential pitfalls of having the wrong or different versions of them locally.
I especially like Volta as a node version manager, as it will change node versions automatically for you based on engines defined in package.json.
Requesting changes as I could not build this locally with the current package.json file.
.gitignore
Outdated
| @@ -7,3 +7,8 @@ relative-json-pointer.txt | |||
|
|
|||
| # For the Python enviornment | |||
| .venv | |||
|
|
|||
| package-lock.json | |||
There was a problem hiding this comment.
I'd have thought we very much wanted to include the package-lock.json file.
What's the reason for git ignoring it?
There was a problem hiding this comment.
That shouldn't be there. I'll remove it. My philosophy is that package-lock should be checked-in for applications, but not for libraries. Most of the code I work on is for libraries, so that got added by mistake due to force of habit.
package.json
Outdated
| "dotenv": "*", | ||
| "eslint": "*", | ||
| "eslint-import-resolver-node": "*", | ||
| "eslint-plugin-import": "*" |
There was a problem hiding this comment.
These should be version specific.
There was a problem hiding this comment.
I've come to believe that pinning versions is a bad practice, especially for dev dependencies. It gets you stuck on out-dated and often abandoned and insecure dependencies. I want my dependencies to be evergreen. Not pinning versions does mean that things will break from time to time, but that's a good thing. When breaking changes are introduced, I want my project to break so I have to deal with them immediately rather than allowing those changes to pile up making it much harder to deal with later.
Our current blog site is a perfect example of this problem. Its dependencies are many years out of date. Many are no longer supported and have unresolved security vulnerabilities. I've tried and failed a couple of times to update the dependencies with the intention of resolving as many security vulnerabilities as I can. Fixing one thing breaks another and so on until I'm blocked by something or the changes just get to be too much. If the dependencies had been updated to the latest as they changed, it would have probably been relatively easy to make the necessary changes because they would have come in one at a time and have been relatively small and not entangled with other changes.
The approach I've been using on my projects for the last few years is to use * for dev dependencies and versions with the ^ prefix for dependencies. I'm really happy with how this has been working out. My builds rarely break and when they do, it's always been a simple fix. I always have the latest features available to me and security vulnerabilities are always quickly and easily resolved just by updating my dependencies.
Let me know if you're willing to give this approach a try. If not, I'll add some versions.
There was a problem hiding this comment.
I think using major version fix (^) makes senes.
I'm VERY aprehensive about not pinning (or at least major version pin) dev dependencies.
If the dependencies had been updated to the latest as they changed...
I've personally encountered a situation where updating a depencency was impossible, after multiple days of trying to figure out why. Turned out, they released an update a few days after we decided to stop trying to fix a compatibility bug. We would do this activity on a monthly basis. Our test coverage was OK.
I expect at some point I will suggest we use Prettier. They continue to strongly recommend exact version pinning, because updated versions could result in different code formatting, which would fail validation checking done in a remote action/build step.
Not pinning versions does mean that things will break from time to time, but that's a good thing.
I disagree. A PR which adds a new article or fixes a typo shouldn't fail to build because one of the dev dependencies has made a new release. Say several dev dependencies are updated at once. We now do not know which is causing the break, and we don't know which version was "good".
Many of the security related issues for the website have only been a concern for sites which have a backend or user created content, so have been ignored. This will still be the case, but we can be better at updating dependencies when they have security issues. GitHub's Dependabot will make upgrade PRs for us to review.
There was a problem hiding this comment.
I'm VERY aprehensive about not pinning (or at least major version pin) dev dependencies.
Ok. I'll add major version pinning. I think it creates a risk for the long-term health of the build, but it's a sufficiently small project that it shouldn't be a significant issue.
A PR which adds a new article or fixes a typo shouldn't fail to build because one of the dev dependencies has made a new release.
This wouldn't happen. Dependencies don't get updated by surprise. We have to update them and they get pinned by package-lock.json (thank you for catching my mistake there). If an update breaks something, fix the typo first, then address the dependency issue.
Say several dev dependencies are updated at once.
That kind of thing would be statistically unlikely if you're updating regularly. (Dependabot can help with that regularity.) When updates happen regularly, breaking changes are rare and more than one package being involved in a breaking change is so rare that I've never seen it in the years I've been working this way (continuous integration). That only happens when trying to update dependencies that haven't been updated regularly for years. Yes, it can happen, but even if there's a break that's difficult to resolve, it's still the best thing to resolve it asap. If you pin it and ignore it, the problem doesn't go away, it just gets harder to resolve later as more dependencies change. Then it doesn't end up getting fixed and you end up with mountains of technical debt that can't be fixed just like our blog site. If you have a dependency that's regularly causing problems, you're better off considering alternatives for that dependency rather than allowing it to bring down the health of your application.
package.json
Outdated
| @@ -0,0 +1,36 @@ | |||
| { | |||
| "name": "json-schema-org/json-schema-spec", | |||
There was a problem hiding this comment.
Yarn refuses to run install, package.json name contains illegal characters.
If we want to use the org prefix, it needs to start with an @.
I think we should, even though we don't (I assume) plan to publish anything on NPM.
| "name": "json-schema-org/json-schema-spec", | |
| "name": "@json-schema-org/json-schema-spec", |
There was a problem hiding this comment.
That was a typo. Good catch. But, yeah, it doesn't really matter what's there because we won't be publishing anything anyway.
There was a problem hiding this comment.
Yarn refuses to run install
I intended npm to be used for this build, but I guess it should work with alternative build tools as well. (Personally, I've never come across a reason to use anything other than npm.)
There was a problem hiding this comment.
That's fair, it's more a side effect. Yarn does some additional pre-work validation. It was also picked up in VSCode as a validaiton error... using JSON Schema, of course!
I think that in the long run, continuous integration is better than avoiding dependency breaks, but not everyone is comforatble with that approach.
|
Thanks for updating this @jdesrosiers. I'll aprove and leave it up to you to make that change or not for before merging the PR. |
Resolves: #1335
I used @yakimun's conversion tool to convert the current core and validation specs to markdown and replaced the old XML versions.
I included the build script I worked on for the proof-of-concept. Details are in the updated README in this PR. The styling is something I hacked together and will need the attention of real web designer at some point.
Known Issues:
Future Work: