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

Resolves issue #19 and #18 #20

Merged
merged 5 commits into from
Oct 31, 2019
Merged

Resolves issue #19 and #18 #20

merged 5 commits into from
Oct 31, 2019

Conversation

abetusk
Copy link
Contributor

@abetusk abetusk commented Oct 30, 2019

  • Rotates mask
  • takes out spurious 'f' characters that somehow got introduces into the argparams.py file

@BernardZhao
Copy link
Collaborator

Thanks for this! I forgot to rotate the mask as well. Solves #19.

As for the f characters, those are Python f-Strings, which allows for the expressions in the brackets to be evaluated, and they aren't actually random. This is a feature in Python 3.6 and above, which is why #18 gave an error.

@abetusk
Copy link
Contributor Author

abetusk commented Oct 30, 2019

I see. The Python3 f-Strings break what would otherwise be a functioning script for (on my system) Python 2.7.12 and Python 3.5.2.

Can we either agree to take the f-Strings out for now or put in something else to ensure some semblance of backwards compatibility?

@BernardZhao
Copy link
Collaborator

Sure, I'm completely fine with just using normal string formatting to ensure backwards compatibility. I'm glad you brought this up, since at the time when I suggested the change, I didn't get any input from others, so I just went ahead and used them.

We can replace all the f-strings with 'Hello, {}'.format(name) formatting, and it should work with both Python 2 and 3.

@abetusk
Copy link
Contributor Author

abetusk commented Oct 31, 2019

ok, there should be an updated version with correct formatting.

I couldn't trigger the case in verify_args that logs a warning about the default output path (line 99) but otherwise I hope it's ok.

Are we waiting on @satyarth to merge?

I should have been paying attention more closely but I only tend to invest in bug fixing and other traffic when I actually use this tool so that's why I'm only noticing now. In the future I'll try and be better about staying on top of what's going on.

Maybe we should get a gallery of people using this tool so we can encourage people to pay attention more...

@satyarth satyarth merged commit e43ee84 into satyarth:master Oct 31, 2019
@satyarth
Copy link
Owner

Looks good, thanks @abetusk and @BernardZhao !

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.

None yet

3 participants