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

Take a dictionary instead of a file #7

Closed
aw-was-here opened this issue Apr 3, 2021 · 11 comments · Fixed by #8
Closed

Take a dictionary instead of a file #7

aw-was-here opened this issue Apr 3, 2021 · 11 comments · Fixed by #8
Assignees
Labels
enhancement New feature or request

Comments

@aw-was-here
Copy link

It would be fantastic if there was a way to replace copyright. Given the limitations of the version field, it is useful to stuff that field with more information at build-time. Thanks!

@DudeNr33
Copy link
Owner

DudeNr33 commented Apr 3, 2021

Can you please give a concrete example of what you want to achieve?
Copyright itself can be set in the YAML file with LegalCopyright. Do you want to set this value from the command line?

@DudeNr33 DudeNr33 added the enhancement New feature or request label Apr 3, 2021
@aw-was-here
Copy link
Author

I'm actually directly calling pyinstaller_versionfile.create_file_version.MetaData as an API so that I can set things programmatically when pyinstaller runs. But it just occurred to me that what I probably need to do is send an IO steam that contains my content rather than have a file on the side.

@DudeNr33
Copy link
Owner

DudeNr33 commented Apr 3, 2021

I will take a look at this - so for you it would be enough if the MetaData class would accept a file-like object instead of a filepath?

@aw-was-here
Copy link
Author

aw-was-here commented Apr 3, 2021

With a bit more playing, with the 1.0.2 version, this hack demonstrates what I'm looking to do.

@aw-was-here
Copy link
Author

So really: rather than taking a raw file or a stream, taking a structure would be much more flexible.

@aw-was-here aw-was-here changed the title Provide a way to replace copyright Take a dictionary instead of a file Apr 3, 2021
@DudeNr33 DudeNr33 self-assigned this Apr 4, 2021
@DudeNr33
Copy link
Owner

DudeNr33 commented Apr 5, 2021

@aw-was-here I pushed a update to the develop branch. For issue #6 I wanted to separate the input of the metadata and creation of the output file anyway, and this should now also make what you want to do straightforward, as you can just create your own MetaData object directly.

Here would be a minimal example:

from pyinstaller_versionfile.metadata import MetaData
from pyinstaller_versionfile.writer import Writer

metadata = MetaData(version="1.2.3.4", legal_copyright="My Imaginary Company")  # all parameters are optional, just specify what you need
writer = Writer(metadata)
writer.render()
writer.save("versionfile.txt")

Would this solve your problem?
Note: this new version would become a V2.0.0 and will also drop support for Python < 3.6.

@aw-was-here
Copy link
Author

Would this solve your problem?

@DudeNr33 Just tried out the develop branch: yes! definitely! This change makes it much easier to incorporate into the build.

Two things probably worth mentioning:

  1. It looks like version validation/fixing isn't actually happening with < 4 digits. I'd expect either a ValueError exception or for the code to just automatically fix version if it isn't 4 decimal places. e.g.:
metadata = MetaData(version='1.2.3', ...)

ends up being

filevers=(1,2,3),

which is obviously wrong.

  1. This usage feels awkward, given I'm not sure what else I'd use the Writer object for:
writer = Writer(metadata)
writer.render()
writer.save("versionfile.txt")

Should save() just call render()? Should render() return the rendered text as well? Or perhaps you have other plans already and it will make more sense later. 👍

Note: this new version would become a V2.0.0 and will also drop support for Python < 3.6.

That's fine for me. I've already got code that requires 3.8 in use.

Thanks!

@DudeNr33
Copy link
Owner

DudeNr33 commented Apr 5, 2021

Thank you for your feedback!

Ad 1)
The MetaData class has two additional methods, that you may call from your calling code:

  • validate() - checks for invalid input. A version with less than 4 decimal places is not considered invalid, as the second method would take care of that:
  • sanitize() - sanitizes the input, i.e. trimming whitespace and filling a version with less than four places with zeros.

I considered calling both methods directly in the __init__ method of the class. But I thought that if somebody imports and uses the code directly, he would probably also like a bit more control over what is happening, so I decided against it.

Ad 2)
render() takes care of rendering the output and handling rendering errors.
save() takes care of the saving the file and handling I/O errors.
I found it cleaner to separate those concerns into individual methods.

I think both issues could be addressed by providing a functional API in the __init__.py of the package, which would essentially implement the logic currently in main.py:main() for the two cases of file-based input and dictionary (or keyword arguments) input.

# __init__.py

def create_versionfile(version, company_name, ..., outfile):
    metadata = MetaData(version, company_name, ...)
    metadata.validate()
    metadata.sanitize()
    writer = Writer(metadata)
    writer.render()
    writer.save(outfile)

def create_versionfile_from_file(infile, outfile, version=None):
    metadata = MetaData.from_file(infile)
    metadata.validate()
    if version:
        metadata.set_version(version)
    metadata.sanitize()
    writer = Writer(metadata)
    writer.render()
    writer.save(outfile)

This would at least save you from calling all methods yourself in the correct order.
Having a second opinion is always great, so fell free to let me know what you think.

@aw-was-here
Copy link
Author

That all looks fantastic. Combined with pyinstaller taking the version file as a parameter these changes make it all very easy to do now.

Thanks!

@DudeNr33
Copy link
Owner

DudeNr33 commented Apr 6, 2021

V2.0.0 is now available on PyPI. Check the updated readme for the final function signature. 👍

@aw-was-here
Copy link
Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants