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

v1.0.0 JSON Schema Update #53

Merged
merged 7 commits into from
Oct 31, 2021
Merged

Conversation

anthonyjdella
Copy link
Contributor

This updates the latest version of LinkedIn-to-JSONResume to use the official v1.0.0 JSON schema.

I've tested it locally and it looks good to me.

@joshuatz
Copy link
Owner

joshuatz commented Oct 8, 2021

Just wanted to say that this is on my radar, but I've been really busy lately and haven't had a chance to give this a look yet. Hopefully within the next week or so I can review this.

@thomasdavis
Copy link

@anthonyjdella good stuff! not a colab on this project but thanks for the effort

@joshuatz joshuatz changed the base branch from main to v1-schema October 31, 2021 07:46
@joshuatz
Copy link
Owner

joshuatz commented Oct 31, 2021

@anthonyjdella Sorry this has taken me a while to get to. I appreciate you making the PR, especially since I haven't touched this for a while, and would not have known that the v1 of the schema was finally released.

I'm going to merge this as-is, into a feature branch, and afterwards make some tweaks before merging into main and eventually releasing. I have to do this because I can't push commits to this PR (see "nitpicks" below for details).

The biggest change I will be making to your code is renaming the different schema options. Now that v1 is not only live on the main branch in the JSON Resume repo, but also featured on the website, in SchemaStore, and elsewhere, I don't think it makes sense to keep calling the older version (v0.0.16) "stable" - I think instead this should be marked as the "legacy" option, and v1 should be considered stable. A lot of this is on me; I should have picked better names for these to begin with.

Here is how I'm changing the options:

Previously
  • Stable: v0.0.16
  • Latest: v0.1.3
  • Beta: v0.1.3 + certificates
Your PR
  • Stable: v0.0.16
  • Latest: v1.0.0
  • Beta: v0.1.3 + certificates
My Proposed Change
  • Legacy: v0.0.16
  • Stable: v1.0.0
  • Beta: Even with v1.0.0 (for now)

Nitpicks

While overall this was a great PR, and I appreciate the changes, I just want to point out some small issues that had to be resolved, and should be avoided in the future:

  • Don't commit changes to package-lock.json unless you have actually updated dependencies.
    • Changes to this file represent a security risk, and are something I have to review 😬
  • Try to make use of rebasing and/or cherry-picking before opening a PR, if your Git history is not "clean"
    • In this PR, I see that you originally had this as your own fork, and updated the README to reflect this. When you opened the PR, you added commit(s) to undo these changes. Normally, I would recommend rebasing so something like an addition and then a deletion is no longer even in the history, period. Or, cherry-pick the stuff you want the maintainer to merge into a separate PR branch, avoiding including your own local changes.
      • This keeps the source repo's Git history cleaner, and also helps avoid accidental "stragglers" - e.g., you accidentally included a change to the README in the form of a new blank line.
    • I'm going to use a squash commit to merge this PR, which will get around this issue, but I would always prefer that PRs have a clean git history (and I think you will find it benefits your own experience too!)
  • Try to make PRs from forks, not copies. Or, really the more important part is make sure that the maintainer has push access to your upstream repository (see: "Allowing changes to a pull request branch created from a fork")
    • This is why I changed the target branch of this PR to a feature branch instead of directly into main - I need to make some changes to your code, but can't do that in this PR because of the permissions on your branch.

@anthonyjdella
Copy link
Contributor Author

@joshuatz Thanks for taking a look and responding! I like the version names that you're proposing, that makes total sense.

Thanks for the info you included!! This was actually my first PR in GitHub (I usually use GitLab), hence the chaos of this PR 😁 . Also I'm not really familiar with NPM or Javascript, so I appreciate the tips!

PS. Awesome work on this project, I think it's really cool!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants