Skip to content

fix optional test locations #382

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

Closed
wants to merge 5 commits into from

Conversation

karenetheridge
Copy link
Member

  • move format tests to their own directory, for consistency
  • move non-format tests out of the format directory that were put there by mistake

@karenetheridge karenetheridge requested review from Julian and a team May 31, 2020 23:45
..except ECMA regex tests that don't use the "format" keyword
They were moved here by mistake in #365.
@karenetheridge karenetheridge force-pushed the ether/fix-optional-test-locations branch from dd7b10a to e2248ce Compare June 3, 2020 02:55
@Julian
Copy link
Member

Julian commented Jun 3, 2020

These kinds of changes are backwards incompatible and intrusive, so we should be careful with them, but I've been aware of the need for this particular one since the folder structure was added to later drafts (truthfully it was probably best in retrospect to not have done that, but too late)...

Will review this carefully but yeah we should be careful with this kind of change in general.

@karenetheridge
Copy link
Member Author

This is mostly to fix a mistake I made a few weeks ago in #365 - not all the tests were "format"-related, doh!

@Julian
Copy link
Member

Julian commented Jun 4, 2020

Sorry -- I clicked the button a bit too quick there -- I'd rather not split up the ecmascript-regex tests -- they all should live in one place I think. It's one unit of functionality (whether an implementation supports or doesn't support an ECMA regex engine). It's true something can support that but not support format, but that's exceedingly unlikely I think.

So would rather we move the entirety of that file back where it was before, including the format-relevant pieces, so that it's all in once spot. Thoughts welcome though.

@karenetheridge
Copy link
Member Author

I have no strong feelings either way. Do you want them all under format/, or outside of it?

I'm not even sure if those ECMA 262 tests are accurate -- @ChALkeR had something to say about that recently.

@Julian
Copy link
Member

Julian commented Jun 4, 2020

I'd prefer outside it I think, just by a bit (since that's how it was "before").

And yes merging this will conflict with @ChALkeR's PR but they'll forgive us :) we can fix the merge conflict if need be, before or after review.

@karenetheridge
Copy link
Member Author

ok, done. I'll clean up the commits and rebase to master when it's approved.

@karenetheridge karenetheridge requested review from Julian and a team June 5, 2020 21:01
@karenetheridge karenetheridge requested review from Julian and a team June 7, 2020 01:02
@karenetheridge karenetheridge deleted the ether/fix-optional-test-locations branch June 8, 2020 22:39
Julian added a commit to python-jsonschema/jsonschema that referenced this pull request Jun 9, 2020
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