From 1f4a79403e192a4c5b02168cfdfeb5d5700fe021 Mon Sep 17 00:00:00 2001 From: Martchus Date: Tue, 13 Mar 2018 19:20:41 +0100 Subject: [PATCH] Turn most warnings into fatal errors --- application/main.cpp | 5 ++- cli/helper.cpp | 95 ++++++++++++++++++++++---------------------- cli/mainfeatures.cpp | 45 +++++++++++++-------- cli/mainfeatures.h | 1 + tests/cli.cpp | 4 +- 5 files changed, 81 insertions(+), 69 deletions(-) diff --git a/application/main.cpp b/application/main.cpp index ffe64ac..b8478de 100644 --- a/application/main.cpp +++ b/application/main.cpp @@ -67,6 +67,7 @@ SetTagInfoArgs::SetTagInfoArgs(Argument &filesArg, Argument &verboseArg) , valuesArg("values", 'n', "specifies the values to be set") , outputFilesArg("output-files", 'o', "specifies the output files; if present, the files specified with --files will not be modified") , backupDirArg("temp-dir", '\0', "specifies the directory for temporary/backup files", { "path" }) + , layoutOnlyArg("layout-only", 'l', "confirms layout-only changes") , setTagInfoArg("set", 's', "sets the specified tag information and attachments") { docTitleArg.setCombinable(true); @@ -140,7 +141,7 @@ SetTagInfoArgs::SetTagInfoArgs(Argument &filesArg, Argument &verboseArg) indexPosValueArg.setImplicit(true); indexPosValueArg.setRequired(true); indexPosArg.setCombinable(true); - indexPosArg.setExample(PROJECT_NAME " set comment=\"with faststart (enforced)\" --index-pos front --force -f /some/dir/*.m4a"); + indexPosArg.setExample(PROJECT_NAME " set comment=\"with faststart\" --index-pos front --force --layout-only -f /some/dir/*.m4a"); indexPosArg.setSubArguments({ &indexPosValueArg, &forceIndexPosArg }); forceRewriteArg.setCombinable(true); valuesArg.setValueNames({ "title=foo", "album=bar", "cover=/path/to/file" }); @@ -163,7 +164,7 @@ SetTagInfoArgs::SetTagInfoArgs(Argument &filesArg, Argument &verboseArg) setTagInfoArg.setSubArguments({ &valuesArg, &filesArg, &docTitleArg, &removeOtherFieldsArg, &treatUnknownFilesAsMp3FilesArg, &id3v1UsageArg, &id3v2UsageArg, &id3InitOnCreateArg, &id3TransferOnRemovalArg, &mergeMultipleSuccessiveTagsArg, &id3v2VersionArg, &encodingArg, &removeTargetArg, &addAttachmentArg, &updateAttachmentArg, &removeAttachmentArg, &removeExistingAttachmentsArg, &minPaddingArg, - &maxPaddingArg, &prefPaddingArg, &tagPosArg, &indexPosArg, &forceRewriteArg, &backupDirArg, &verboseArg, &outputFilesArg }); + &maxPaddingArg, &prefPaddingArg, &tagPosArg, &indexPosArg, &forceRewriteArg, &backupDirArg, &layoutOnlyArg, &verboseArg, &outputFilesArg }); } } // namespace Cli diff --git a/cli/helper.cpp b/cli/helper.cpp index ccacdc7..d9f9b59 100644 --- a/cli/helper.cpp +++ b/cli/helper.cpp @@ -264,7 +264,9 @@ TimeSpanOutputFormat parseTimeSpanOutputFormat(const Argument &timeSpanFormatArg } else if (!strcmp(val, "seconds")) { return TimeSpanOutputFormat::TotalSeconds; } else { - cerr << Phrases::Warning << "The specified time span format \"" << val << "\" is invalid and will be ignored." << Phrases::EndFlush; + cerr << Phrases::Error << "The specified time span format \"" << val << "\" is invalid." << Phrases::End + << "note: Valid formats are measures, colons and seconds." << endl; + exit(-1); } } return defaultFormat; @@ -281,7 +283,9 @@ TagUsage parseUsageDenotation(const Argument &usageArg, TagUsage defaultUsage) } else if (!strcmp(val, "always")) { return TagUsage::Always; } else { - cerr << Phrases::Warning << "The specified tag usage \"" << val << "\" is invalid and will be ignored." << Phrases::EndFlush; + cerr << Phrases::Error << "The specified tag usage \"" << val << "\" is invalid." << Phrases::End + << "note: Valid values are never, keepexisting and always." << endl; + exit(-1); } } return defaultUsage; @@ -301,7 +305,9 @@ TagTextEncoding parseEncodingDenotation(const Argument &encodingArg, TagTextEnco return TagTextEncoding::Utf16LittleEndian; } else if (!strcmp(val, "auto")) { } else { - cerr << Phrases::Warning << "The specified encoding \"" << val << "\" is invalid and will be ignored." << Phrases::EndFlush; + cerr << Phrases::Error << "The specified encoding \"" << val << "\" is invalid." << Phrases::End + << "note: Valid encodings are utf8, latin1, utf16be, utf16le and auto." << endl; + exit(-1); } } return defaultEncoding; @@ -318,7 +324,9 @@ ElementPosition parsePositionDenotation(const Argument &posArg, const Argument & } else if (!strcmp(val, "keep")) { return ElementPosition::Keep; } else { - cerr << Phrases::Warning << "The specified position \"" << val << "\" is invalid and will be ignored." << Phrases::EndFlush; + cerr << Phrases::Error << "The specified position \"" << val << "\" is invalid." << Phrases::End + << "note: Valid positions are front, back and keep." << endl; + exit(-1); } } return defaultPos; @@ -334,8 +342,8 @@ uint64 parseUInt64(const Argument &arg, uint64 defaultValue) return stringToNumber(arg.values().front()); } } catch (const ConversionException &) { - cerr << Phrases::Warning << "The specified value \"" << arg.values().front() << "\" is no valid unsigned integer and will be ignored." - << Phrases::EndFlush; + cerr << Phrases::Error << "The specified value \"" << arg.values().front() << "\" is no valid unsigned integer." << Phrases::EndFlush; + exit(-1); } } return defaultValue; @@ -350,7 +358,9 @@ TagTarget::IdContainerType parseIds(const std::string &concatenatedIds) try { convertedIds.push_back(stringToNumber(id)); } catch (const ConversionException &) { - cerr << Phrases::Warning << "The specified ID \"" << id << "\" is invalid and will be ignored." << Phrases::EndFlush; + cerr << Phrases::Error << "The specified ID \"" << id << "\" is invalid." << Phrases::End << "note: IDs must be unsigned integers." + << endl; + exit(-1); } } return convertedIds; @@ -363,8 +373,9 @@ bool applyTargetConfiguration(TagTarget &target, const std::string &configStr) try { target.setLevel(stringToNumber(configStr.substr(13))); } catch (const ConversionException &) { - cerr << Phrases::Warning << "The specified target level \"" << configStr.substr(13) << "\" is invalid and will be ignored." - << Phrases::EndFlush; + cerr << Phrases::Error << "The specified target level \"" << configStr.substr(13) << "\" is invalid." << Phrases::End + << "note: The target level must be an unsigned integer." << endl; + exit(-1); } } else if (configStr.compare(0, 17, "target-levelname=") == 0) { target.setLevelName(configStr.substr(17)); @@ -378,8 +389,8 @@ bool applyTargetConfiguration(TagTarget &target, const std::string &configStr) target.attachments() = parseIds(configStr.substr(17)); } else if (configStr.compare(0, 13, "target-reset=") == 0) { if (*(configStr.data() + 13)) { - cerr << Phrases::Warning << "Invalid assignment " << (configStr.data() + 13) << " for target-reset will be ignored." - << Phrases::EndFlush; + cerr << Phrases::Error << "Invalid assignment " << (configStr.data() + 13) << " for target-reset." << Phrases::EndFlush; + exit(-1); } target.clear(); } else if (configStr == "target-reset") { @@ -408,13 +419,11 @@ FieldDenotations parseFieldDenotations(const Argument &fieldsArg, bool readOnly) const auto fieldDenotationLen = strlen(fieldDenotationString); if (!strncmp(fieldDenotationString, "tag=", 4)) { if (fieldDenotationLen == 4) { - cerr << Phrases::Warning - << "The \"tag\"-specifier has been used with no value(s) and hence is ignored. Possible values are: " - "id3,id3v1,id3v2,itunes,vorbis,matroska,all" - << Phrases::EndFlush; + cerr << Phrases::Error << "The \"tag\"-specifier has been used with no value(s)." << Phrases::End + << "note: Possible values are id3,id3v1,id3v2,itunes,vorbis,matroska and all." << endl; + exit(-1); } else { TagType tagType = TagType::Unspecified; - bool error = false; for (const auto &part : splitString(fieldDenotationString + 4, ",", EmptyPartsTreat::Omit)) { if (part == "id3v1") { tagType |= TagType::Id3v1Tag; @@ -432,19 +441,14 @@ FieldDenotations parseFieldDenotations(const Argument &fieldsArg, bool readOnly) tagType = TagType::Unspecified; break; } else { - cerr << Phrases::Warning - << "The value provided with the \"tag\"-specifier is invalid and will be ignored. Possible values are: " - "id3,id3v1,id3v2,itunes,vorbis,matroska,all" - << Phrases::EndFlush; - error = true; - break; + cerr << Phrases::Error << "The value \"" << part << " for the \"tag\"-specifier is invalid." << Phrases::End + << "note: Possible values are id3,id3v1,id3v2,itunes,vorbis,matroska and all." << endl; + exit(-1); } } - if (!error) { - scope.tagType = tagType; - scope.allTracks = false; - scope.trackIds.clear(); - } + scope.tagType = tagType; + scope.allTracks = false; + scope.trackIds.clear(); } continue; } else if (applyTargetConfiguration(scope.tagTarget, fieldDenotationString)) { @@ -454,7 +458,6 @@ FieldDenotations parseFieldDenotations(const Argument &fieldsArg, bool readOnly) bool allTracks = false; vector trackIds; trackIds.reserve(parts.size()); - bool error = false; for (const auto &part : parts) { if (part == "all" || part == "any") { allTracks = true; @@ -463,19 +466,14 @@ FieldDenotations parseFieldDenotations(const Argument &fieldsArg, bool readOnly) try { trackIds.emplace_back(stringToNumber(part)); } catch (const ConversionException &) { - cerr << Phrases::Warning - << "The value provided with the \"track\"-specifier is invalid and will be ignored. It must be a comma-separated list " - "of track IDs." - << Phrases::EndFlush; - error = true; - break; + cerr << Phrases::Error << "The value provided with the \"track\"-specifier is invalid." << Phrases::End + << "note: It must be a comma-separated list of track IDs." << endl; + exit(-1); } } } - if (!error) { - scope.allTracks = allTracks; - scope.trackIds = move(trackIds); - } + scope.allTracks = allTracks; + scope.trackIds = move(trackIds); continue; } @@ -511,9 +509,8 @@ FieldDenotations parseFieldDenotations(const Argument &fieldsArg, bool readOnly) fileIndex += static_cast(*digitPos - '0') * mult; } if (!fieldNameLen) { - cerr << Phrases::Warning << "Ignoring field denotation \"" << fieldDenotationString << "\" because no field name has been specified." - << Phrases::EndFlush; - continue; + cerr << Phrases::Error << "The field denotation \"" << fieldDenotationString << "\" has no field name." << Phrases::EndFlush; + exit(-1); } // parse the denoted field ID @@ -525,9 +522,9 @@ FieldDenotations parseFieldDenotations(const Argument &fieldsArg, bool readOnly) } } catch (const ConversionException &e) { // unable to parse field ID denotation -> discard the field denotation - cerr << Phrases::Warning << "The field denotation \"" << string(fieldDenotationString, fieldNameLen) - << "\" could not be parsed and will be ignored: " << e.what() << Phrases::EndFlush; - continue; + cerr << Phrases::Error << "The field denotation \"" << string(fieldDenotationString, fieldNameLen) + << "\" could not be parsed: " << e.what() << Phrases::EndFlush; + exit(-1); } // read cover always from file @@ -540,8 +537,9 @@ FieldDenotations parseFieldDenotations(const Argument &fieldsArg, bool readOnly) // add value to the scope (if present) if (equationPos) { if (readOnly) { - cerr << Phrases::Warning << "Specified value for \"" << string(fieldDenotationString, fieldNameLen) << "\" will be ignored." - << Phrases::EndFlush; + cerr << Phrases::Error << "A value has been specified for \"" << string(fieldDenotationString, fieldNameLen) << "\"." << Phrases::End + << "note: This is only possible when the \"set\"-operation is used." << endl; + exit(-1); } else { // file index might have been specified explicitely // if not (mult == 1) use the index of the last value and increase it by one if the value is not an additional one @@ -552,8 +550,9 @@ FieldDenotations parseFieldDenotations(const Argument &fieldsArg, bool readOnly) } } if (additionalValue && readOnly) { - cerr << Phrases::Warning << "Indication of an additional value for \"" << string(fieldDenotationString, fieldNameLen) - << "\" will be ignored." << Phrases::EndFlush; + cerr << Phrases::Error << "Indication of an additional value for \"" << string(fieldDenotationString, fieldNameLen) << "\" is invalid." + << Phrases::End << "note: This is only possible when the \"set\"-operation is used." << endl; + exit(-1); } } return fields; diff --git a/cli/mainfeatures.cpp b/cli/mainfeatures.cpp index c1e2e33..aab0e8c 100644 --- a/cli/mainfeatures.cpp +++ b/cli/mainfeatures.cpp @@ -144,7 +144,7 @@ void displayFileInfo(const ArgumentOccurrence &, const Argument &filesArg, const // check whether files have been specified if (!filesArg.isPresent() || filesArg.values().empty()) { cerr << Phrases::Error << "No files have been specified." << Phrases::End; - return; + exit(-1); } MediaFileInfo fileInfo; @@ -306,7 +306,7 @@ void displayTagInfo(const Argument &fieldsArg, const Argument &filesArg, const A // check whether files have been specified if (!filesArg.isPresent() || filesArg.values().empty()) { cerr << Phrases::Error << "No files have been specified." << Phrases::End; - return; + exit(-1); } // parse specified fields @@ -365,11 +365,11 @@ void setTagInfo(const SetTagInfoArgs &args) // check whether files have been specified if (!args.filesArg.isPresent() || args.filesArg.values().empty()) { cerr << Phrases::Error << "No files have been specified." << Phrases::EndFlush; - return; + exit(-1); } if (args.outputFilesArg.isPresent() && args.outputFilesArg.values().size() != args.filesArg.values().size()) { cerr << Phrases::Error << "The number of output files does not match the number of input files." << Phrases::EndFlush; - return; + exit(-1); } // get output files @@ -384,7 +384,15 @@ void setTagInfo(const SetTagInfoArgs &args) && (!args.removeAttachmentArg.isPresent() || args.removeAttachmentArg.values().empty()) && (!args.docTitleArg.isPresent() || args.docTitleArg.values().empty()) && !args.id3v1UsageArg.isPresent() && !args.id3v2UsageArg.isPresent() && !args.id3v2VersionArg.isPresent()) { - cerr << Phrases::Warning << "No fields/attachments have been specified." << Phrases::End; + if (!args.layoutOnlyArg.isPresent()) { + cerr << Phrases::Error << "No fields/attachments have been specified." << Phrases::End + << "note: This is usually a mistake. Use --layout-only to prevent this error and apply file layout options only." << endl; + exit(-1); + } + } else if (args.layoutOnlyArg.isPresent()) { + cerr << Phrases::Error << "Fields/attachments and --layout-only have been specified." << Phrases::End + << "note: Don't use --layout-only if you actually want to alter tag information or attachments." << endl; + exit(-1); } TagCreationSettings settings; @@ -412,8 +420,8 @@ void setTagInfo(const SetTagInfoArgs &args) } else if (applyTargetConfiguration(targetsToRemove.back(), targetDenotation)) { validRemoveTargetsSpecified = true; } else { - cerr << Phrases::Warning << "The given target specification \"" << targetDenotation << "\" is invalid and will be ignored." - << Phrases::End; + cerr << Phrases::Error << "The given target specification \"" << targetDenotation << "\" is invalid." << Phrases::EndFlush; + exit(-1); } } } @@ -427,8 +435,9 @@ void setTagInfo(const SetTagInfoArgs &args) } } catch (const ConversionException &) { settings.id3v2MajorVersion = 3; - cerr << Phrases::Warning << "The specified ID3v2 version \"" << args.id3v2VersionArg.values().front() - << "\" is invalid and will be ingored." << Phrases::End; + cerr << Phrases::Error << "The specified ID3v2 version \"" << args.id3v2VersionArg.values().front() << "\" is invalid." << Phrases::End + << "note: Valid versions are 1, 2, 3 and 4." << endl; + exit(-1); } } @@ -505,13 +514,15 @@ void setTagInfo(const SetTagInfoArgs &args) if (segmentIndex < segmentCount) { container->setTitle(newTitle, segmentIndex); } else { - cerr << Phrases::Warning << "The specified document title \"" << newTitle - << "\" can not be set because the file has not that many segments." << Phrases::End; + diag.emplace_back(DiagLevel::Warning, + argsToString( + "The specified document title \"", newTitle, "\" can not be set because the file has not that many segments."), + context); } ++segmentIndex; } } else { - cerr << Phrases::Warning << "Setting the document title is not supported for the file." << Phrases::End; + diag.emplace_back(DiagLevel::Warning, "Setting the document title is not supported for the file.", context); } } @@ -761,12 +772,12 @@ void extractField( } if (((fieldDenotations.size() != 1) || (!attachmentInfo.hasId && !attachmentInfo.name)) && ((fieldDenotations.size() == 1) && (attachmentInfo.hasId || attachmentInfo.name))) { - cerr << Phrases::Error << "Excactly one field or attachment needs to be specified." << Phrases::End; - return; + cerr << Phrases::Error << "Excactly one field or attachment needs to be specified." << Phrases::EndFlush; + exit(-1); } if (!inputFilesArg.isPresent() || inputFilesArg.values().empty()) { - cerr << Phrases::Error << "No files have been specified." << Phrases::End; - return; + cerr << Phrases::Error << "No files have been specified." << Phrases::EndFlush; + exit(-1); } MediaFileInfo inputFileInfo; @@ -900,7 +911,7 @@ void exportToJson(const ArgumentOccurrence &, const Argument &filesArg, const Ar // check whether files have been specified if (!filesArg.isPresent() || filesArg.values().empty()) { cerr << Phrases::Error << "No files have been specified." << Phrases::End; - return; + exit(-1); } RAPIDJSON_NAMESPACE::Document document(RAPIDJSON_NAMESPACE::kArrayType); diff --git a/cli/mainfeatures.h b/cli/mainfeatures.h index e0f7e6b..aea9391 100644 --- a/cli/mainfeatures.h +++ b/cli/mainfeatures.h @@ -41,6 +41,7 @@ struct SetTagInfoArgs { ApplicationUtilities::Argument valuesArg; ApplicationUtilities::Argument outputFilesArg; ApplicationUtilities::ConfigValueArgument backupDirArg; + ApplicationUtilities::ConfigValueArgument layoutOnlyArg; ApplicationUtilities::Argument setTagInfoArg; }; diff --git a/tests/cli.cpp b/tests/cli.cpp index e6a60c6..80f3d1f 100644 --- a/tests/cli.cpp +++ b/tests/cli.cpp @@ -845,7 +845,7 @@ void CliTests::testFileLayoutOptions() string stdout, stderr; const string mp4File1(workingCopyPath("mtx-test-data/alac/othertest-itunes.m4a")); - const char *const args1[] = { "tageditor", "set", "--tag-pos", "back", "--force", "-f", mp4File1.data(), nullptr }; + const char *const args1[] = { "tageditor", "set", "--tag-pos", "back", "--force", "--layout-only", "-f", mp4File1.data(), nullptr }; TESTUTILS_ASSERT_EXEC(args1); const char *const args2[] = { "tageditor", "info", "-f", mp4File1.data(), nullptr }; @@ -877,7 +877,7 @@ void CliTests::testFileLayoutOptions() != string::npos); remove((mp4File2 + ".bak").data()); - const char *const args6[] = { "tageditor", "set", "--index-pos", "front", "--force", "-f", mp4File2.data(), nullptr }; + const char *const args6[] = { "tageditor", "set", "--index-pos", "front", "--force", "--layout-only", "-f", mp4File2.data(), nullptr }; TESTUTILS_ASSERT_EXEC(args6); TESTUTILS_ASSERT_EXEC(args4);