Urs' first suggestions #1
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Moving the yaml import directly to the parsing of a config file makes python3-yaml an optional dependency - you only need it *if* you use the config file.
a) The path should be handled like a regular shell path variable, i.e. using the colon as a separator. b) When there is only one path it should be wrapped in a one-element list, otherwise an error condition will occur in _executables() in the list comprehension (which will iterate over the option's *characters*) But actually the path handling should be changed generally. I think (if I'm not mistaken) the paths given should be *base* paths from which all child directories with lilypond installations can be inferred (both binary releases and local builds, with their differing directory structures).
Using os.path.splitext() avoids hardcoding the '.ly' extension
a) refactor code to use an inner function
(making the code more modular and (possibly) better maintainable)
b) also check for existence of input file(s)
This is insufficient, as I realized there is a problem with -dsomething options (see entry in Issues.md)
This is a first step towards a separation of CLI program and Python module
(forgot to do this in 96477ae)
This completely changes the way the path options are handled to determine installed binaries. From each given path possible binaries are searched - in locations matching binary releases and custom builds - with the given path seen as - an installation dir or - a parent dir for multiple installations - the directory containing the 'lilypond' executable
uliska
commented
Feb 22, 2017
lydiff/lyversions.py
Outdated
| else: | ||
| self._list = [] | ||
| self._paths = paths.split(os.pathsep) if os.pathsep in paths else [paths] | ||
| self._list = self._executables() if search else [] |
There was a problem hiding this comment.
This gives me the opportunity to ask for which use case this search is for
lydiff.configure() is newly factored out to the lydiff module. - ability to reuse this in a standalone program and in Frescobaldi - separate configuration, checks and feedback
... so they can be used indpendently from the CLI script
When input files are entered using relative paths (i.e. not either in the current dir or specified as absolute paths) all the functions failed. This is treated by determining the absolute path of the input files and composing all relevant file names as absolute paths. (Change handling of file extensions as well)
Before executing the tools all temporary files matching the current setting and the resulting diff file are removed. This is to avoid the situation that some process failed (e.g. LilyPond couldn't produce a score) but intermediate files from previous runs hide that fact. After the completion the temporary files (without the diff file) are removed, if the -k (--keep-intermediate-files) option isn't set. Note: This doesn't handle multi-page scores yet.
Don't rely on the originally generated file names but look for png files in the directory after LilyPond has run. Compare all the pages with their correspondents, if one score has more pages simply copy the trailing ones
NOTE: the pdftk stuff has not been implemented yet.
If one or both LilyPond versions fail to produce a score an informative error message is printed (instead of the obscure FileNotFoundError) and the script is aborted. Note that this is based on the previous change to purge old files before compilation. Note also that we can't rely on LilyPond's exit code because LilyPond already "fails" when the \version is too old.
Oversight in 649e4f6: Purging directories only looked for the actual outfile, so if the previous run produces multiple diff files they were not detected
Implement basic classes and instantiate a LyDiff object with the CLI options in place.
Not all functions have been tested yet (because currently they are not reached yet)
Move helper functions to the top, add a number of doc strings.
With the new -i option *only* the LilyPond versions found below the path (default or given) are displayed and nothing else performed. As a change to be discussed this changes the positional "files" argument to the -f/--files one (because otherwise one would have to add a dummy file to make -i work)
Shulyaka
reviewed
Sep 11, 2020
| if diff_file is None: | ||
| if equal(self.input_files): | ||
| diff_file = 'diff_%s_%s-%s' % (self.input_basenames[0], *self.lily_versions) | ||
| elif equal(target_versions): |
There was a problem hiding this comment.
Suggested change
| elif equal(target_versions): | |
| elif equal(self.target_versions): |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is not a coherent pull request but a collection of individual suggestions. They can be merged, improved and merged or chosen and cherry-picked individually.