Skip to content

Add a basic JSON validation CI action#6

Closed
peternewman wants to merge 6 commits into
ssilverman:masterfrom
peternewman:patch-4
Closed

Add a basic JSON validation CI action#6
peternewman wants to merge 6 commits into
ssilverman:masterfrom
peternewman:patch-4

Conversation

@peternewman

Copy link
Copy Markdown
Contributor

No description provided.

@peternewman peternewman mentioned this pull request Jun 27, 2021

@ssilverman ssilverman left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could you combine the commits into one?

@peternewman

Copy link
Copy Markdown
Contributor Author

@peternewman

Copy link
Copy Markdown
Contributor Author

Is the Squash and Merge option acceptable @ssilverman or do you want me to have a go from my end?

@ssilverman

Copy link
Copy Markdown
Owner

I'm familiar with squash merges, I just wanted to give you an opportunity to create your own commit message. If you don't want to, I'll just create a commit and give you credit in the comments.

@peternewman

Copy link
Copy Markdown
Contributor Author

I believe if you do them from GitHub then I think it takes the Pull Request message as the message (or something equally clever/sensible) and deals with the credit too, I think we both get credited or it shows me as the author in collaboration with you or vice versa so it still properly shows my authorship, not just as a comment in the commit message.

Basically I'd rather you just did that, and it's less effort all round I believe.

I'll avoid writing the same thing on the other PRs...

@ssilverman

ssilverman commented Jul 28, 2021

Copy link
Copy Markdown
Owner

That's true, but I think it also puts all the temporary commits into the message too. This is a single-file one-change commit that's in 4 5 6 commits. Could you combine them into one?

@peternewman peternewman changed the title Add some basic JSON validation Add a basic JSON validation CI action Jul 28, 2021
@ssilverman

Copy link
Copy Markdown
Owner

Here's the workflow that I'd like for this repo (I should really put this into a contributor's guide):

  1. I'm using a rebase approach for linear history. (I'm used to working on very large codebases with lots of people, and so my habits are from that.)
  2. Combine multi-commit changes that really are only one change into a single commit.
  3. In the PRs, use a single commit per "thing", and put unrelated changes into different PRs.
  4. Force push as necessary into your own branch to do the commit combining, so that the rebase-merge into the main branch keeps the above points.

@peternewman

Copy link
Copy Markdown
Contributor Author

That's true, but I think it also puts all the temporary commits into the message too. This is a single-file one-change commit that's in 4 5 6 commits. Could you combine them into one?

I've just tried a test squash and merge, my PR looked like this:
peternewman#8

The resulting squash and merge commit looks like this:
peternewman@e72a09a

As you'll see you can sub/copy edit the commit message to your hearts content to fit your preferred workflow.

Here's the workflow that I'd like for this repo (I should really put this into a contributor's guide):

Yes you should, if you use a magic name, GitHub does lots of clever stuff when it shows it to people (although I think frustratingly only after you've made an edit and open a PR...):
https://docs.github.com/en/communities/setting-up-your-project-for-healthy-contributions/setting-guidelines-for-repository-contributors

1. I'm using a rebase approach for linear history. (I'm used to working on very large codebases with lots of people, and so my habits are from that.)

I'd say the OLA codebase is pretty big, and we've got away with it, I don't really know how it compares to what you've worked on, anyway it's your repo so you're welcome to run it as you want.

2. Combine multi-commit changes that really are only one change into a single commit.

I think squash will do that. I've done 95% of the edits to this repo/open PRs solely on the GitHub website, rather than downloading and installing all the random tools, which unfortunately translates to a lot of commits, but does mean a very low barrier to entry.

3. In the PRs, use a single commit per "thing", and put unrelated changes into different PRs.

Makes sense, you may want to clarify if things like my typo fix which is independent of adding the action are unrelated or not, or how literally you want to take that if I see a typo in a file I'm editing...

4. Force push as necessary into your own branch to do the commit combining, so that the rebase-merge into the main branch keeps the above points.

I'm not certain, but I think one of the GitHub workflows will cover that for you.

@@ -0,0 +1,9 @@
name: Validate JSONs Alternative

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

What do you mean by "JSONs Alternative"?

@peternewman peternewman Aug 9, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just that it was one of a number of ways of doing the validation. My first was https://github.com/peternewman/rdm-schema/tree/validation which still seems to fail ( https://github.com/peternewman/rdm-schema/runs/3279250852?check_suite_focus=true ), I think possibly as the validator is older than the schema you're now using.

Given the various online and CLI validators seem to catch different things, running a suite of them probably makes sense, but maybe I should name them based on which tool is doing the validation for each run.

@peternewman peternewman mentioned this pull request Aug 9, 2021
@peternewman

Copy link
Copy Markdown
Contributor Author

Closing this in favour of #8

@peternewman peternewman closed this Aug 9, 2021
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.

2 participants