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

Change json array to dictionary #2530

Closed
wants to merge 1 commit into from
Closed

Change json array to dictionary #2530

wants to merge 1 commit into from

Conversation

Freed-Wu
Copy link
Contributor

jq can only sort json dictionary. It is because json array is ordered which should not be sorted. I add a zero-width match to fix bug of \faFile*

jq can only sort json dictionary. It is because json array is ordered which should not be sorted. I add a zero-width match to fix bug of `\faFile*`
@lervag
Copy link
Owner

lervag commented Oct 27, 2022

Well, we don't need jq here, do we? We could just use a simple thing like sort?

Further, it seems we spend a lot of effort in adding this check for something that will 1) rarely be changed, and 2) if it changes, I will still be aware it should be sorted and will take care to ensure it stays sorted.

So, perhaps an alternative here is to just remove the workflow?

@Freed-Wu
Copy link
Contributor Author

We could just use a simple thing like sort

sort cannot sort json dictionary.

echo '{"b": 1,"a": 1}'|jq -S .
{
  "a": 1,
  "b": 1
}
❯ echo '{"b": 1,"a": 1}'|sort
{"b": 1,"a": 1}

an alternative here is to just remove the workflow

This PR is only related to a regular expr. The PR about workflow is #2531.

@lervag
Copy link
Owner

lervag commented Oct 27, 2022

sort cannot sort json dictionary.

echo '{"b": 1,"a": 1}'|jq -S .
{
  "a": 1,
  "b": 1
}
❯ echo '{"b": 1,"a": 1}'|sort
{"b": 1,"a": 1}

No, but sort (with some more shell scripting) can sort the json array.

an alternative here is to just remove the workflow

This PR is only related to a regular expr. The PR about workflow is #2531.

Yes. I'll think some more about this later today.


I'm still skeptical of changing this back to a dictionary. The regex becomes harder to read and more complicated, and there really seems to be no real benefit in my opinion. That is, the only benefit is to allow the workflow to check that the json file is sorted. But the reason to check that it is sorted is precisely what you fix with the more complicated regex here.

Still, perhaps having it as a dictionary makes sense in other ways, so I'll reconsider later today.

lervag added a commit that referenced this pull request Oct 27, 2022
@lervag
Copy link
Owner

lervag commented Oct 27, 2022

Ok, I've merged this while making a few minor changes:

  • I slightly altered the regex for a minor improvemnet to clarity.
  • I removed the assets.yml workflow as it is no longer necessary (we don't need to care about the file being sorted).

@lervag lervag closed this Oct 27, 2022
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