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

Fix JSON file formatting #784

Merged
merged 1 commit into from
Sep 12, 2024
Merged

Conversation

colinleach
Copy link
Contributor

This is purely the result of running bin/configlet fmt -u -y.

The changes are largely cosmetic. The most important thing is they remove the noise from configlet fmt output, which should now show any new problems without having to scroll through 79 old ones.

@colinleach
Copy link
Contributor Author

Should this be marked as [no important files changed]? That's something I've not dealt with before, and need to get my head around.

@depial
Copy link
Contributor

depial commented Sep 12, 2024

Should this be marked as [no important files changed]? That's something I've not dealt with before, and need to get my head around.

I suspect it doesn't need that. Here's a bit of the docs which gives some explanation, but in short the [no important files changed] seems to help "avoid triggering unnecessary test runs".

@colinleach colinleach enabled auto-merge (squash) September 12, 2024 20:15
@colinleach colinleach requested a review from depial September 12, 2024 20:15
@colinleach
Copy link
Contributor Author

@depial, do you have authority to approve this, or do we need a cross-track-maintainer?

Copy link
Contributor

@depial depial left a comment

Choose a reason for hiding this comment

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

lgtm

@colinleach colinleach merged commit 54d676a into exercism:main Sep 12, 2024
11 checks passed
@colinleach
Copy link
Contributor Author

thanks!

@depial
Copy link
Contributor

depial commented Sep 12, 2024

Cheers!

If anything else is sufficiently reviewed that we can merge, you can feel free to let me know. I'm still not used to having merging powers, so I'm still a little hesitant to use them unprompted

@colinleach
Copy link
Contributor Author

I'm still not used to having merging powers

I know the feeling!

@depial
Copy link
Contributor

depial commented Sep 18, 2024

Edit: Found it... There is an extra Leap in concepts.wip which is likely the culprit. I would assume it's not showing up on the overview tab since config.json only has one entry for it.

@colinleach I've just noticed that there seems to be two versions of the Leap exercise on the website. Can you see this too or is it something with my browser?

From what I can tell, they are clones, with the only difference I can see being the blurb (though I only made a cursory exploration). Even all of the solutions seem to be the same (at least I assume they are the same since they both have 69 pages of solutions and the same top solutions)

I can't see anything in the .json files that would be responsible for that, but I'm leaving the message here because this is my best guess as to what could have lead to it.

Comparative weirdness:

  • On the overview tab there is only one version and total exercise count is 95
  • On the practice tab there are two versions and total exercise count is 96

@colinleach
Copy link
Contributor Author

This is a mess, not of our making.

If you look at it in an incognito window, everything is good. As maintainers, we're seeing a Leap clone because the previous maintainers wanted to use this as a Concept exercise. This is strictly forbidden by Jeremy, who insists on keeping Concept and Practice exercises totally separate. I hear rumors that there have been bad-tempered arguments about this in the past, which he refuses to repeat (and he's the CEO, so tends to have the last word).

The files are in exercises/concept.wip. My recommendation is to submit a PR to remove that duplicate Leap directory. Even if we somehow wanted to use it, we wouldn't be allowed. Rightly, IMHO.

@depial
Copy link
Contributor

depial commented Sep 18, 2024

Now that you mention that, it reminds me of an open PR #365. It looks like they planned to rename it and then deprecate it, but that was almost 3 1/2 years ago. I don't know how much of that process is still relevant (e.g. Do we have to deprecate something or can we simply remove it?). Also, the file mentioned in the PR, exercises/concept/leap/, is no longer there (presumably moved into concept.wip)

@colinleach
Copy link
Contributor Author

colinleach commented Sep 18, 2024

Just deleting it should be OK.

Closing #365 may also be a good idea.

@depial
Copy link
Contributor

depial commented Sep 19, 2024

I'll try to take a look at doing that tomorrow.

Closing #365 may also be a good idea.

I feel like there's quite a few old PRs that are probably able to be closed as obsolete, but I'm not sure if it's the norm to keep them around for some reason

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