mute in ~/.tidyrc runs fine but triggers exit(1) #752
Comments
|
@dse had to build a special Windows tidy, add That problem is that the service used to read the config file, in This will bump the This it the patch to begin the fix in Windows - diff --git a/src/config.c b/src/config.c
index f5e8379..085f4b6 100644
--- a/src/config.c
+++ b/src/config.c
@@ -877,8 +877,26 @@ static ctmbstr ExpandTilde( TidyDocImpl* doc, ctmbstr filename )
if (filename[1] == '/')
{
home_dir = getenv("HOME");
- if ( home_dir )
+ if (home_dir) {
++filename;
+ }
+#ifdef _MSC_VER
+ else if (strlen(filename) >= 3) { /* at least '~/+1' */
+ /* no HOME env in Windows - got for HOMEDRIVE=C: HOMEPATH=\Users\user */
+ char * hd = getenv("HOMEDRIVE");
+ char * hp = getenv("HOMEPATH");
+ if (hd && hp) {
+ ctmbstr s = TidyDocAlloc(doc, _MAX_PATH);
+ strcpy(s, hd);
+ strcat(s, hp);
+ strcat(s, "\\");
+ strcat(s, &filename[2]);
+ return s;
+ }
+
+ }
+#endif /* _MSC_VER */
+
}
#ifdef SUPPORT_GETPWNAM
elseBut that does nothing about the error output Need to understand more... this happens in |
|
@geoffmcl I'm having a hard time following the code. But while I'm working on that could you look into using something other than |
|
@ler762 thanks for looking at the patch... in this case that could probably be Ok, this is NOT just the If using Command line option uses Still digging... any pointers very welcome... |
|
@geoffmcl the config file checking leaves much to be desired. I'm still trying to follow the code to see how hard it would be to fix, but check this out - I added a printf right after the I don't get an error for a totally bogus line in ~/.tidyrc: It doesn't even mention the existence of I'm not sure where to go with this - just have wrt the off-topic ifdef comment It'd be nice if a windows build of tidy could be easily done using gcc. |
|
@ler762 thanks for the feedback... I do not understand Yes, we are talking about an API service, tidyLoadConfig, which advertises So agree there maybe a logic question here... Chain: Why does output of a But as you point out, if the config file contains a bad option, like So this points to at least, an adjustment/enhancement in the Maybe something like In the 3 cases shown in the above code, the failed to open the file, would already be reported, so could be just But the problem I see is that the Alternatively, these 3 additional error message outputs could be removed from For the API docs, suggest at least - diff --git a/include/tidy.h b/include/tidy.h
index c701890..c1bbd41 100644
--- a/include/tidy.h
+++ b/include/tidy.h
@@ -481,16 +481,16 @@ TIDY_EXPORT void TIDY_CALL tidyGeneralInfo( TidyDoc tdoc );
/** Load an ASCII Tidy configuration file and set the configuration per its
- ** contents.
- ** @result Returns 0 upon success, or any other value if there was an error.
+ ** contents. Reports config option errors, which can be filtered.
+ ** @result Returns 0 upon success, or any other value if there was an option error.
*/
TIDY_EXPORT int TIDY_CALL tidyLoadConfig(TidyDoc tdoc, /**< The TidyDoc to which to apply the configuration. */
ctmbstr configFile /**< The complete path to the file to load. */
);
/** Load a Tidy configuration file with the specified character encoding, and
- ** set the configuration per its contents.
- ** @result Returns 0 upon success, or any other value if there was an error.
+ ** set the configuration per its contents. Reports config option errors, which can be filtered.
+ ** @result Returns 0 upon success, or any other value if there was an option error.
*/
TIDY_EXPORT int TIDY_CALL tidyLoadConfigEnc(TidyDoc tdoc, /**< The TidyDoc to which to apply the configuration. */
ctmbstr configFile, /**< The complete path to the file to load. */Ideas, feedback welcome... OT: Just checked in my msys2 bash prompt and 1. HOME is defined, so the WIN32 code would not be required, and 2. the tilde has already been expanded, so the WIN32 code would not be required. But maybe there is a case for a native mingw-x64 build, like |
|
PS: Actually the 3 cases in |
|
@geoffmcl after looking at the code much more, maybe ignoring the return code or having consider:
Considering the comment that Which would leave only the issue of ifdef OT: compiler defines: |
|
@ler762 - Thanks for looking deeper into the code... the best clue was to look at Agree 100% with Using a html input, Now Maybe that is ok... then trying Is this message a But this does not address the config error cases... like I can now see if the line does not contain a colon, Have pushed some of these fixes to the Still exploring more here... feedback very welcome... OT: hope closed ;=)) |
|
@geoffmcl I'm glad you figured it out - I was still struggling. I did a build from your issue-752 branch - question: how to supress the Info: message?
Yes, looks good. Thank you :) |
|
@ler762 again thanks for checking out
A quick answer is, put as the first line, in the config file, Remember these, potentially 3 config files, are read BEFORE the command line is parsed, so nothing on it, like It seems an error like At present, an option error will cause And if you are directly using So I am suggesting And no one reponded on whether the line like Looks like we already have a typo correction, PR #753... thanks... Waiting for further feedback... |
To remember, one has to have know at some time in the past :) Wow! This bug report has opened a can of worms: So the man page needs clarification that adds the specified config file for options processing and does not replace or prevent processing of ~/.tidyrc I haven't checked the regression test docs, but it probably needs something about having a ~/.tidyrc invalidates the testing
No, it should not be silently ignored; it should generate a warning.
Sounds good, but it kind of makes it harder to debug if there's more than one options config file. It'd be nice if tidy told you which config file had the problem. I don't have time to work on this today - hopefully tomorrow I'll be able to work on it & submit a matching change to the regression test. |
|
@geoffmcl The more I think about it the less I like tidy not processing command line options first. What do you/anyone think about having tidy process the command line options first and allowing them to over-ride default behaviors? If nothing else, it'd be nice to be able to run the regression tests without having to make sure ~/.tidyrc doesn't exist |
|
@ler762 sometimes, perhaps, you need to think a little more before you leap... but thank you for your feedback ... here and there ;=))
I am mainly a Windows person, but in the unix app world that seems one thing clear... if there exists a In running the regression tests, at least in windows, there is a test for ENV So agree 100%, before running the regression tests, check for such a config files existance, otherwise all bets are off on the tests succeeding... many of which depend on a very specific config environment of their own being established... simply, must be no pre-command line configuration file loaded, silently, mostly... unless there are errors, some of which are also silent... This suggests a PR in tests... in Just saying, maybe not time to adjust the tests just yet... but thanks for testing and running... and the results... So working on regression test changes before this bug is solved seems time not well spent... but agree it is important... to see, to know, the impact on this or that change... as it evolves... so, thanks... We need to finalize this issue... get it accepted... discussed/agreed.. merged into If there are some tests update required, then that is the time for a PR's on that... Need more feedback on this can of worms issue... |
|
@geoffmcl Yes, I oppose having a
It seems like most unix utilities allow you to over-ride processing the .XXXrc file.
Neither. Windows used to win on ease of use + software availability & cygwin made it a pretty nice platform, but I just can't accept Windows 10. The last version of Unix I liked was SunOS, so that should date me pretty well :) Linux seems to like chasing the latest shiny whatever.. altho Debian stable seems pretty good at long-term support, so that's what I'm trying now
envars always seemed like a better-than-nothing workaround. I don't much like 'em, but I've used them when there wasn't some other way.
I really liked Nancy Reagans' "Just Say NO" suggestion. So I said no to building & testing if ~/.tidyrc existed: At this stage of the game, I'd suggest documenting the issue & leaving a better fix for later.
I liked your issue-752 branch; I think that should go into the next release.
I'd leave off fixing the parsing of .tidyrc & changing/adding command line options processing for later. Anything I left off? |
|
@geoffmcl take a look at #754. I think this removes the need for most of the documentation changes.
no longer needed. Tidy processes the
no longer needed. The regression tests specify a See why I don't like having a ${HOME}/.XXXrc file that can't be over-ridden? :) |
|
@ler762 well this issue seems to have morphed into a Feature Request to alter the pre-command line config file reading, present since the birth of tidy, or at least the 2000 source I have, seemingly just because of your don't like ... file that can't be over-ridden ;=)) While some unix apps do have an over-ride option, many do not. I know at least one app that will create such a file, even if one does not exist... so it does seem a choice, which But for starters, with Also, except for some new options, like And I have no problem informing a tidy regression tester that they should carefully check before running the tests... they are not strictly casual user tests... such So I for one, am against PR #754 altering this part of Look forward to further comments on the |
|
@geoffmcl Isn't that the way open-source works -- don't like ... make better :-) Considering your earlier comment about having to build a special windows tidy & enhance the code, I can sort of see why you wouldn't want to change anything; this doesn't affect you. But the standard tidy build on *nix defines That tidy has been doing it this way since forever carries about as much weight with me as
I have a problem with that, because if the order of processing is changed to cli options first, it's not necessary. No work-arounds needed, no code changes to add a If tidy is changed to do cli options first & read one init file if the user doesn't give a |
|
@ler762 sorry if I found the reasoning of don't like just a little amusing... while don't like is probably the first driver that causes me too to look for change, I will try to back it up with valid facts, reasoning, ... and present those, leaving out the purely emotive don't like argument as implicit, implied... but maybe just me... I am all for change if it improves And in open source, where there are choices, sometimes what one person likes can be disliked by others... even used as a loving, expected, features of the app by some, or ignored, or worked around by others... sometimes generating So it seems here the question is something like The creators, and all maintainers since then, chose to read certain config files before processing the command line, as the norm... as [m]any good apps do... And in all that I can read around, that seems a valid choice still for today, for an 18+ year old, Now @ler762 suggests we change that... maybe he makes some points... but none that sway me that it is a clear improvement to I could argue that if I take the steps to create a But that I can also add one, or more, That is what Look forward to some comments from others... time to change, or not? |
|
@geoffmcl no problem; don't like was what got me here in the first place asking for a change :) In any case, I've been thinking more about the reasons for why I don't like..
OK - take a look at *nix users have automatically processed init files by default. Windows users don't see the problem because there are no automatically processed init files on windows unless you go out of your way to enable them. I'm in the *nix user category, so I do have a problem. For example, the tidy regression tests fail because I have a ~/.tidyrc (I was both surprised & pleased to discover that #754 fixed that :) A bit far-fetched, but consider what happens if someone decides to go along with Or what happens to an end-user whose sysadmin decides to create an I help out on a project that uses tidy to clean up docbook generated html. I can't even remember to do a In short, #754 enables *nix users to have their kate and edith too :) echoing Geoff's request: |
|
Still waiting for feedback on my PR #764 which addresses several issues brought up... namely add windows tilde expansion 94e62b2, plus 8c3ef4b, 4ba6106, ... etc... @ler762, these seem valid regardless of the of the I think for the Hope to get around to merging PR #764, and some others, soon... still marching towards maybe a 6.0.0 release, maybe for new year... |
If I specify a value for the
muteoption in~/.tidyrc, for example:This causes
tidyto work normally, observing themuteoption I specified, but it also causestidyto output the following error message and exit with a return code of 1:Specifying
--mute MISSING_DOCTYPEon the command line instead of specifying it in~/.tidyrcdoes not trigger this problem.Reproducing is simple enough. Run the following command with
mute: MISSING_DOCTYPEin~/.tidyrc:Then take that line out of
~/.tidyrcthen run the following:The text was updated successfully, but these errors were encountered: