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

bug: pgn headers write in double due to forgotten regex~ness of string #51

Closed
jzacsh opened this issue Jan 18, 2014 · 3 comments
Closed

Comments

@jzacsh
Copy link
Contributor

jzacsh commented Jan 18, 2014

bug description copy/pasted from jzacsh@232e5d0

var chess = new Chess();
chess.move({from: 'e2', to: 'e4'});
var pgn = chess.pgn({max_width: 5, newline_char: '\n'});
// output:
// [White "carolyn"]
// [Black "hippo"]
// [Date "2014-1-18"]
// 
// 1. e4

var chessB = new Chess();
chessB.load_pgn(pgn);

// BUG HERE:
chessB.pgn({max_width: 5, newline_char: '\n'});
// outputs: [[White "carolyn"] [Black "hippo"] [Date "2014-1-18"] "[White "carolyn"] [Black "hippo"] [Date "2014-1-18"]"]  1. e4

test added that confirms this behavior in: jzacsh@fa55217

jzacsh added a commit to jzacsh/chess.js that referenced this issue Jan 18, 2014
@jzacsh
Copy link
Contributor Author

jzacsh commented Jan 18, 2014

IMO: it would make more sense from an API/user perspective to pick a clear data type to be passed for the .split() call, and avoid using a string that looks strangely like (but not quite) a RegExp.

The string required (and internally defaulted to) is: '\r?\n'
Instead, the API should either:

  • require a regex-valid string like it seems to be attempting, ie: user would pass: '\\r?\\n'
  • require a RegExp object, ie: user would pass the result of /\r?\n/ or new RegExp('\r?\n')

As it is right now, there's just more scraping that has to take place (the mask function that tries to escape the API input -- which doesn't seem like a necessary pain).

jzacsh added a commit to jzacsh/chess.js that referenced this issue Jan 18, 2014
jhlywa added a commit that referenced this issue Jan 27, 2014
fix for issue(#51) - writing double `[[headers "foo"] headers "foo"]`
@jzacsh
Copy link
Contributor Author

jzacsh commented Jan 28, 2014

If you want me to update the API per my last comment, feel free to reopen this issue and I'll happily send you another patch!

@jzacsh jzacsh closed this as completed Jan 28, 2014
@ghost
Copy link

ghost commented Aug 18, 2018

I know this is really old, but I was perusing this code for the first time and came across the mask function:
function mask(str) { return str.replace(/\\/g, '\\'); }
Please correct me if I'm wrong, but it looks like this function does absolutely nothing and may as well just return str itself. Is there any reason to keep this mask() function at all?

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

No branches or pull requests

1 participant