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

Adds {journal} field to file options and tests it #65

Open
wants to merge 6 commits into
base: pr-perfect-undo
Choose a base branch
from

Conversation

boyanpenkov
Copy link

wip

dontmerge

closes #64

I am looking at adding the {journal} option here, and thing I have tracked though the entry mechanism there. however, when I run, I see the following error still:

(python) → more_fields Repos/papers papers add ../../Literature/Stage/2013_AdvCIS_Modeling\ and\ simulation\ of\ electrostatically\ gated\ nanochannels.pdf
Traceback (most recent call last):
  File "/home/boyan/miniconda3/envs/python/bin/papers", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/home/boyan/Vazhno/Work/Repos/papers/papers/__main__.py", line 1195, in main
    check_install(subp, o, config) and addcmd(subp, o, config)
                                       ^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/boyan/Vazhno/Work/Repos/papers/papers/__main__.py", line 534, in addcmd
    biblio.add_pdf(file, attachments=o.attachment, rename=o.rename, copy=o.copy,
  File "/home/boyan/Vazhno/Work/Repos/papers/papers/bib.py", line 435, in add_pdf
    self.insert_entry(entry, update_key=True, **kw)
  File "/home/boyan/Vazhno/Work/Repos/papers/papers/bib.py", line 288, in insert_entry
    self.insert_entry_check(entry, update_key=update_key, rename=rename, copy=copy, **checkopt)
  File "/home/boyan/Vazhno/Work/Repos/papers/papers/bib.py", line 345, in insert_entry_check
    file = merge_files([candidate, entry], relative_to=self.relative_to)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/boyan/Vazhno/Work/Repos/papers/papers/duplicate.py", line 290, in merge_files
    check = checksum(f) if os.path.exists(f) else None
            ^^^^^^^^^^^
  File "/home/boyan/Vazhno/Work/Repos/papers/papers/utils.py", line 109, in checksum
    return hash_bytestr_iter(file_as_blockiter(open(fname, 'rb')), hashlib.sha256())
                                               ^^^^^^^^^^^^^^^^^
IsADirectoryError: [Errno 21] Is a directory: '/home/boyan/Vazhno/Work/Literature'

@perrette -- am I missing something here? That thing is indeed a directory, which is the target for the PDFs I'd like to have.

I will end up testing this in test_add.py unless there are objections.

@perrette
Copy link
Owner

perrette commented Jul 3, 2024

Without looking in detail, the items can be stored as name.pdf , or as name/original_name.pdf, to allow for attachments to be saved alongside (it's quite, but not entirely PDF-centric).

@perrette
Copy link
Owner

perrette commented Jul 3, 2024

...and the checksum function can only apply to a PDF, not to a directory

@boyanpenkov
Copy link
Author

@perrette -- Ok, ready for your perspective... with a significant caveat:

The actual feature works, and the test to check is test_add_rename_copy_journal() in test_add.py. This passes, and manually inspecting the behavior on a few files passes as well, so that's nice.

The bad thing is that the test suite runs papers directly, and so depends on the config the user has specified (which, in my case for for testing this, does have {journal} and so breaks the temp files that are written.) This breaks a lot of the undo-redo tests; all breakages there have been marked TODO and I know what they are.

What might get around this is generating an --ignore-install option, for papers to simply not read the install config and run with the consistent defaults. Alternately, are you familiar with https://docs.pytest.org/en/6.2.x/fixture.html ? Couching the tests in terms of these might be a more robust long-term solution, but is a re-work of more major sorts.

So, I think I need an opinion from the papers BDFL here. ;)

@boyanpenkov
Copy link
Author

@perrette -- just touching base here...

@boyanpenkov
Copy link
Author

Hello, just wanted to touch base here....

@boyanpenkov
Copy link
Author

@perrette -- hello, just reaching out to see what your opinion here is...

Thanks kindly!

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.

2 participants