Skip to content

Adds URL to constructor #9

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ciscorucinski
Copy link

Description of proposed changes

It is intended to allow for #7 url proposal.

This proposal modified the existing constructor to take in required, named arguments. If you don't use the default constructor, then you must specify the argument you are setting Aliasor(alias_file=<file>). This changes the behavior of the existing file argument as you can no longer call Aliasor(<file>).

I added the ability to specify a url via Aliasor(alias_url=<url>).

The use of required, named arguments should be implemented if you want to add different input data such as str (which can then be implemented as Aliasor(alias_string=<str>)). This makes it very clear what the caller wants to do.

Related issue(s)

Testing

I added a test which takes in the default url currently provided, and tested that compress and uncompress logic still works correctly. I also updated the file constructor test to use the new required, named argument implementation.

@ciscorucinski
Copy link
Author

I am using PyCharm, and the current test imports from this project don't work as-is.

I had to change from pango_aliasor.aliasor import Aliasor to from src.pango_aliasor.aliasor import Aliasor to remove import errors and get the tests to be recognized.

I did not include that in this pull request, but if you would like me to, I can add this change, too.

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.

1 participant