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

Strings in Ruby #555

Merged

Conversation

pulgamecanica
Copy link
Contributor

@pulgamecanica pulgamecanica commented Oct 4, 2022

What GitHub issue does this PR apply to?

Resolves #426

What changed and why?

I implemented the Strings module for the Ruby 3.x lang 😄
I used the official Ruby documentation as well as The Well-Grounded Rubyist in case you want to add some references in the future

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
  • (If editing Django) I have added or edited any appropriate unit tests for my changes

@geekygirlsarah geekygirlsarah reopened this Oct 4, 2022
@pulgamecanica pulgamecanica changed the title JSON finished not reviewed Strings in Ruby Oct 4, 2022
@pulgamecanica
Copy link
Contributor Author

Hey! I wish I was more careful, I have one mistake.
I misplaced two questions and I forgot to answer one.

The questions misplaced should be one row above, and I should answer the black space that would be left


Screen Shot 2022-10-04 at 18 52 14


I feel very noob r, wow, srry.

Tbh I feel like, after all, it's good to have PR's instead of committing directly...

@pulgamecanica
Copy link
Contributor Author

Fixed misplaced section and answered the question about alpha strings.

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 looks great! A couple of changes to match some best practices and I'll happily merge this in!

web/thesauruses/ruby/3/strings.json Show resolved Hide resolved
web/thesauruses/ruby/3/strings.json Outdated Show resolved Hide resolved
@pulgamecanica
Copy link
Contributor Author

I will read this before going any further...
https://docs.codethesaur.us/thesaurus/

@pulgamecanica
Copy link
Contributor Author

Hello, I wanted to ask about this behavior...

Whenever I replace "code" for "comment", then the application will display "Unknown" above the comment, this is because the application assumes that "code" exists, but I didn't declare it on the JSON scope, I only declared the "comment".

Screen Shot 2022-10-05 at 11 19 48

There is one way to fix it, which would be to declare the "code" as an empty array on the JSON scope, but this will leave a blank space above the comment.

Screen Shot 2022-10-05 at 11 23 21


Perhaps there should be a condition to check if "code" is empty, and if that's the case, only display the comment line, thus removing the blank space for the empty comment.

@pulgamecanica
Copy link
Contributor Author

I also found out why it says, "Unknown".
I think, if you agree with me, it's a nice change. Whenever the concept IS known, but the "code" is None then return None, instead of "unknown".

I already solved it, if you agree with me I would be more than happy to make this change happen.

@pulgamecanica
Copy link
Contributor Author

My solution would be this:


On the models, on this function:

def concept_code(self, concept_key):

I would change it to something like this:

def concept_code(self, concept_key):
        code = self.concept(concept_key).get("code")
        if not code:
            return None
        if isinstance(code, list):
            code = "\n".join(code)
        return code

This way, whenever the "code" in the JSON object is not defined, you return None. (if not code, return None)


On the views on this function :

def format_code_for_display(concept_key, lang):

I would change it to something like this:

def format_code_for_display(concept_key, lang):
    if lang.concept_unknown(concept_key):
        return "Unknown"
    if lang.concept_code(concept_key) is None:
        return None
    if lang.concept_implemented(concept_key):
        return highlight(
            lang.concept_code(concept_key),
            get_lexer_by_name(lang.key, startinline=True),
            HtmlFormatter()
        )
    return None

So, if the concept is defined but the code is None, then you return None, instead of "Unknown"

@pulgamecanica
Copy link
Contributor Author

This will actually make this line work:

And the div will not be created, because "code" it's returning None...

@pulgamecanica
Copy link
Contributor Author

Hello, I implemented the " comment", and used the "not-implemented": true where I thought it was necessary.

I also fixed the ordered list.


I will open an issue regarding what I described above, I think it's better, that way we can separate these issues.
Best regards,
André

@pulgamecanica
Copy link
Contributor Author

I think I've finished what was left for this String module, I finally deleted the "name" field from the JSON file, implemented the comments; etc as I described before. I would be more than happy to hear your feedback, and I was excited about creating a PR for the "Unkown" issue if you can have a look at it at some point.
Cheers 😀

@geekygirlsarah
Copy link
Member

I feel very noob r, wow, srry.

Tbh I feel like, after all, it's good to have PR's instead of committing directly...

Don't feel too bad. I've definitely committed stuff that broke the project before, lol!

Also a LOT of people have submitted stuff that's not exactly right. Probably at least half! So don't feel bad. It's why the review process and the GitHub Actions are there. To help validate things before they go in.

@geekygirlsarah
Copy link
Member

I'm starting to see a bit better why #558 is there. I'll think on that.

Want to wait on the result of #558, or want to just change "comment" back to "code" and I can pull it in now?

@pulgamecanica
Copy link
Contributor Author

pulgamecanica commented Oct 8, 2022

I can change it back to code and save this JSON for when it's resolved.

@geekygirlsarah
Copy link
Member

That works for me!

@pulgamecanica pulgamecanica force-pushed the issue-426-ruby-add-strings branch from d82f72c to 591c175 Compare October 8, 2022 04:55
@pulgamecanica
Copy link
Contributor Author

I believe it's done :D

@geekygirlsarah
Copy link
Member

This works for me! Thank you for this addition, and your first contribution to CT! 🎉

If you are participating in Hacktoberfest, don't forget to register so that this PR counts towards that.

@geekygirlsarah geekygirlsarah merged commit c7ce132 into codethesaurus:main Oct 9, 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.

[Ruby] Add strings
2 participants