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

Clean up topics vs practices in config.json #1421

Conversation

dmaahs2017
Copy link
Contributor

@dmaahs2017 dmaahs2017 commented Jan 2, 2022

Addresses #1407. I think this also has something to do with #1089. Though I'm not entirely sure.

I also created a python script to automate this task.

@dmaahs2017 dmaahs2017 force-pushed the 1407/clean-topics-vs-practices-in-config branch from b08d96b to 66785c4 Compare January 2, 2022 00:25
@dmaahs2017 dmaahs2017 changed the title 1407 Clean up topics vs practices in config.json Clean up topics vs practices in config.json Jan 2, 2022
Copy link
Member

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Thank you, this is useful work! While I'd slightly prefer it if the script were written in Rust, I do understand the desire for a shorter script for a utility item like this, and it will be fine that it's currently in Python.

bin/clean_topics_vs_practices.py Outdated Show resolved Hide resolved
bin/clean_topics_vs_practices.py Outdated Show resolved Hide resolved
bin/clean_topics_vs_practices.py Outdated Show resolved Hide resolved
bin/clean_topics_vs_practices.py Outdated Show resolved Hide resolved
bin/clean_topics_vs_practices.py Outdated Show resolved Hide resolved
bin/clean_topics_vs_practices.py Outdated Show resolved Hide resolved
@dmaahs2017
Copy link
Contributor Author

dmaahs2017 commented Jan 4, 2022

Thank you, this is useful work! While I'd slightly prefer it if the script were written in Rust, I do understand the desire for a shorter script for a utility item like this, and it will be fine that it's currently in Python.

I do love Rust; it just seemed a bit overkill for this use-case. And thank you for the suggestions! 😃

@dmaahs2017 dmaahs2017 requested a review from coriolinus January 4, 2022 17:26
@dmaahs2017
Copy link
Contributor Author

dmaahs2017 commented Jan 4, 2022

@coriolinus Is this an important issue? If so, how would you like it to be resolved? Is this logic I should add to the script?

image

@coriolinus
Copy link
Member

@coriolinus Is this an important issue?

Not sure, I haven't seen it before. I asked the other maintainers. I'll let you know when I hear back.

@coriolinus
Copy link
Member

The response seems to be that:

  • there isn't yet any documentation I can link to about the restriction, unfortunately
  • but it's a real limitation we can't bypass. It breaks the website's styling if any concept has more than 10 "practices" exercises

To me this sounds like we need a post-processing step. I think it's safe to assume that entries in config.json are sorted in approximate order of difficulty / relevance, so we just need something like

for concept in concepts:
    count = 0
    for practice_exercise in config['exercises']['practice']:
        if concept in practice_exercise['practices']:
            count += 1
        if count > 10:
            practice_exercise['practices'].remove(concept)

@dmaahs2017 dmaahs2017 requested a review from coriolinus January 5, 2022 21:40
@dmaahs2017
Copy link
Contributor Author

Thanks for looking into that. I've worked that logic into the script and re-ran it.

Copy link
Member

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Just two more edits, sorry.

  • If we remove a topic from the practices array due to count, we should put it back into the topics array. That ensures we don't destroy information. I should have thought of that earlier.
  • Both the practices and topics arrays should be sorted for each exercise after all other modifications are complete. The linter will complain otherwise.

Co-authored-by: Peter Goodspeed-Niklaus <[email protected]>
@dmaahs2017 dmaahs2017 force-pushed the 1407/clean-topics-vs-practices-in-config branch from c2e06ff to 40f5dbd Compare January 6, 2022 18:19
@dmaahs2017
Copy link
Contributor Author

Sounds good, added that logic as well

@dmaahs2017 dmaahs2017 requested a review from coriolinus January 6, 2022 18:20
Copy link
Member

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Thanks for doing this work!

@coriolinus coriolinus merged commit 9436629 into exercism:main Jan 6, 2022
@dmaahs2017 dmaahs2017 deleted the 1407/clean-topics-vs-practices-in-config branch January 7, 2022 17:44
@dmaahs2017
Copy link
Contributor Author

Glad I could help! 😃

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