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

added nano; simplify #168

Merged
9 commits merged into from
Oct 5, 2021
Merged

added nano; simplify #168

9 commits merged into from
Oct 5, 2021

Conversation

hexiro
Copy link
Contributor

@hexiro hexiro commented Oct 3, 2021

made some small changes which shouldn't affect logic and added nano.

changes:

  • added nano
  • replaced the 2nd snippet with the 1st
if sys.version_info < (3, 6):
if major < 3 or (major == 3 and minor < 6):
  • made the Wikipedia link in scripts/get_file_sigs.py use https protocol
  • replaced instances of dict() with {}
  • replaced if / elses w/ ternary expressions
  • replaced set([]) w/ {}
  • replaced some for loops with list comprehensions

lmk what you think 👍🏼

"Regex": "^(nano|xrb)_[13]{1}[13456789abcdefghijkmnopqrstuwxyz]{59}$",
"plural_name": false,
"Description": null,
"Rarity": 0.8,
Copy link
Owner

Choose a reason for hiding this comment

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

This is probably 1 because it require having the string nano or xrb in it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i thought so too but I didn't want to over estimate. I can make that change today. What about the 10 checks failing?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Try running pytest on your machine inside of the PyWhat directory, so that tests run and you can see the reason why ones are failing.

And don't forget to run black and isort for your code formatting. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do :)

@bee-san
Copy link
Owner

bee-san commented Oct 4, 2021

Failing because:

>       assert rarity_num == sorted(
            rarity_num, reverse=True
        ), "Regexes should be sorted by rarity in 'regex.json'. Regexes with rarity '1' are at the top of the file and '0' is at the bottom."

@hexiro
Copy link
Contributor Author

hexiro commented Oct 4, 2021

Failing because:


>       assert rarity_num == sorted(

            rarity_num, reverse=True

        ), "Regexes should be sorted by rarity in 'regex.json'. Regexes with rarity '1' are at the top of the file and '0' is at the bottom."

gotcha gotcha that makes sense. Will change when i get the chance :)

@hexiro
Copy link
Contributor Author

hexiro commented Oct 4, 2021

I'll check this out later when I'm on my home pc and make sure there are no more failing tests.
I've never used poetry before but it looks very cool, so thank you for exposing me to it!👍🏼

@hexiro
Copy link
Contributor Author

hexiro commented Oct 4, 2021

does the position in regex.json affect the order of the matches? I left it at the bottom of the rarity: 1s because nano is quite uncommon (thus unlikely to be a false positive)

hexiro added 2 commits October 4, 2021 15:05
thanks to SkeletalDemise and r49behind from bee's discord :)
@hexiro hexiro requested a review from bee-san October 4, 2021 19:07
@bee-san
Copy link
Owner

bee-san commented Oct 5, 2021

Hey! Mind fixing the merge conflict and I can merge? Thanks!!! :)

@hexiro
Copy link
Contributor Author

hexiro commented Oct 5, 2021

merge conflicts gone 🦀
p.s. you might want to squash and merge

@ghost ghost merged commit ac0ffeb into bee-san:main Oct 5, 2021
@hexiro hexiro deleted the Hexiro-patch-1 branch October 5, 2021 11:30
This pull request was closed.
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.

3 participants