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

Fixing issue #746 #751

Merged
merged 2 commits into from
Oct 5, 2024
Merged

Conversation

ayaan-qadri
Copy link
Contributor

What GitHub issue does this PR apply to?

Resolves #746

What changed and why?

There was an issue with Pygments because it does not support 'clips' highlighting. After some research, I found that 'prolog' has a similar syntax highlighting style. Therefore, I modified the get_lexer_by_name function to check for 'clips' and changed it with 'prolog'. I tested the changes, and the issue has been resolved, with syntax highlighting working as expected.

(If editing website code) Please add screenshots

image

127 0 0 1_8000-CodeThesaurus-ReferenceforCLIPS(641)

Checklist

  • I claimed any associated issue(s) and they are not someone else's
  • I have looked at documentation to ensure I made any revisions correctly
  • I tested my changes locally to ensure they work
  • For language files, I have validated the edited files are valid JSON and data shows up correctly
  • For website code edits, I have added or edited any appropriate unit tests for my changes

Any additional comments or things to be aware of while reviewing?

Copy link
Member

@geekygirlsarah geekygirlsarah left a comment

Choose a reason for hiding this comment

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

This is a fix I'm willing to take, but it shouldn't duplicate the effort of #744. Take out those changes and just leave views.py changes and I'll merge this in!

web/thesauruses/_meta/rules_facts.json Outdated Show resolved Hide resolved
web/thesauruses/clips/641/rules_facts.json Outdated Show resolved Hide resolved
web/thesauruses/meta_info.json Outdated Show resolved Hide resolved
@ayaan-qadri
Copy link
Contributor Author

@geekygirlsarah Sure thing, I will just do necessary change which is in view.py asap, thank you !

@ayaan-qadri
Copy link
Contributor Author

Hey @geekygirlsarah , I'm currently checking for the 'clips' language only. Would you like to make Prolog the default for syntax highlighting?

Advantages:
If we set Prolog as the default language, then when we add any new languages in the future that aren't supported by Pygments, this same error won't occur again.

Disadvantages:
Syntax highlighting for new languages can't be guaranteed. It will only work if the new language's syntax is similar to Prolog's.

@geekygirlsarah
Copy link
Member

Would you like to make Prolog the default for syntax highlighting?

I'd say no. Maybe just the plain text lexer instead?

image

@ayaan-qadri
Copy link
Contributor Author

Would you like to make Prolog the default for syntax highlighting?

I'd say no. Maybe just the plain text lexer instead?

image

Sure, then I will set plain text lexer as default if it's not exists.

Copy link
Member

@geekygirlsarah geekygirlsarah left a comment

Choose a reason for hiding this comment

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

Interesting change, I was thinking of moving the lexers to the meta file for languages to keep language stuff together, but I guess it could work in settings too. I can merge it in if you tidy up the try/except blank line!

web/views.py Outdated Show resolved Hide resolved
@ayaan-qadri
Copy link
Contributor Author

So, i created a new function named 'get_highlighter' which returns lexer and added SIMILAR_LEXERS in setings.py so now if this case happens again we just have to add into dictionary <language_which_is_not_support_in_pygments> : <similar_syntax_langauge_supported_in_pygments> if it exists in SIMILAR_LEXERS it will pick that else default it will be plain 'text' lexer.

@ayaan-qadri
Copy link
Contributor Author

Interesting change, I was thinking of moving the lexers to the meta file for languages to keep language stuff together, but I guess it could work in settings too. I can merge it in if you tidy up the try/except blank line!

I chose to put it in settings because the settings are loaded into memory when the app starts, which is comparatively faster than anything else !

Copy link
Member

@geekygirlsarah geekygirlsarah left a comment

Choose a reason for hiding this comment

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

I'll merge it in! Thanks for helping out!

Don't forget it's Hacktoberfest this month. Be sure to register if you haven't as this project counts towards it!

@geekygirlsarah geekygirlsarah merged commit f3f6408 into codethesaurus:main Oct 5, 2024
5 checks passed
@ayaan-qadri ayaan-qadri deleted the Fixing-Issue-#746 branch October 5, 2024 19:41
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.

[Bug] For a language without a Pygments lexer, the program crashes
2 participants