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

Tidy rows longer than 80 chars to separate rows in plain text files #1894

Closed

Conversation

guykisel
Copy link
Contributor

#1891

I would prefer to make the max length a Tidy command line option, but I would have to make a lot of changes to pass that value all the way down to rowsplitter. It seems a lot simpler to just set a max character count in TxtFormatter and pass that into rowsplitter. I would also prefer to pass the column separator into rowsplitter for use in calculating the row length, but there doesn't seem to be a simple way to do that in the current structure. Assuming the separator is four spaces seems to work well enough.

@robotci
Copy link

robotci commented Jan 12, 2015

Tests passed.

Refer to this link for build results (access rights to CI server needed):
http://robot.radiaatto.ri.fi//job/RobotFramework-pull-requests/59/Tests passed.

@jussimalinen
Copy link
Member

Thanks for the pull request! The idea seems very good, but when discussing this with @pekkaklarck we thought this probably be configurable from the command line..

We cant take this into 2.8.7 anymore (planning release for Tomorrow), but lets look at this for 2.9.

@guykisel
Copy link
Contributor Author

Sure, sounds good.

@guykisel
Copy link
Contributor Author

Note this is a work in progress as I update this PR to reflect the latest changes in master and also try to make this configurable from the command line.

@robotci
Copy link

robotci commented Jan 22, 2015

Tests FAILED!

Refer to this link for build results (access rights to CI server needed):
http://robot.radiaatto.ri.fi//job/RobotFramework-pull-requests/64/Tests FAILED!

@robotci
Copy link

robotci commented Jan 22, 2015

Tests FAILED!

Refer to this link for build results (access rights to CI server needed):
http://robot.radiaatto.ri.fi//job/RobotFramework-pull-requests/65/Tests FAILED!

@pekkaklarck
Copy link
Member

Sorry for not looking at this PR earlier. What's the current status?

@guykisel
Copy link
Contributor Author

Haven't looked at this in a few weeks. Making it configurable from the command line is a bit difficult due to how deeply nested the rowsplitter is. I need to spend more time looking at the structure of the relevant code and thinking about it.

@pekkaklarck
Copy link
Member

Yeah, the related code seems to be a bit too clever. Let me know if you want to discuss it e.g. on IRC some day. Before a major release we can do even larger refactoring if needed.

@guykisel
Copy link
Contributor Author

Sure, that could be good. My schedule is a bit uncertain for the next few weeks, so I might not be able to have a live discussion for a while. I don't think this is a high priority enhancement, since I doubt Tidy is heavily used.

@richardsavio
Copy link

I would like to work on this. May I pick it up if it is not being worked on?

@pekkaklarck
Copy link
Member

Please go ahead!

@fabo-at-noona
Copy link
Contributor

there was PR#2771 which has been closed last week since nobody reviewed it.

@pekkaklarck
Copy link
Member

Tidy has been totally rewritten as part of RF 3.2 parser rewrite and there is not way this PR could anymore be merged. Good news is that the new Tidy is a lot better to enhance if needed.

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.

6 participants