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

Don't put array items on the same line when persisting #558

Merged
merged 1 commit into from
May 30, 2016

Conversation

mpeterv
Copy link
Contributor

@mpeterv mpeterv commented May 22, 2016

Rockspec fields with array values (e.g. dependencies) are typically written with each item on its own line. This change makes luarocks new-version follow this style, too. This also affects they way manifests are saved.

@hishamhm
Copy link
Member

IIRC, this style was used because of manifest files. Since manifest files are zipped now, I suppose it makes no difference...

@mpeterv
Copy link
Contributor Author

mpeterv commented May 22, 2016

@hishamhm why? This doesn't change manifest size, both separators have length 2

@hishamhm
Copy link
Member

deeply indented subtables produce lots of spaces

@mpeterv
Copy link
Contributor Author

mpeterv commented May 30, 2016

@hishamhm would you like an option to retain old behaviour when saving manifests or is this OK?

@@ -147,17 +144,11 @@ write_table = function(out, tbl, level, field_order)
end

write_value(out, v, level, sub_order)
if type(k) == "number" then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking at the manifest tables. I suggest this compromise solution: if type(v) == "number". This will catch the version tables correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that for parsed version tables in dependencies table? That could work. Although isn't that table redundant? repository table contains dependencies in string format already...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is redundant, but I wanted to store the pre-parsed info for speeding things up for luarocks.loader (though I don't remember if I measured it, and if I did, it was back in 2007 or so when I first wrote this code). The idea was that luarocks.loader should work as little as possible to have minimal impact on require.

Instead of pairs with number keys. Keeps parsed version representation
compact but puts items in regular arrays each on its own line.
@mpeterv mpeterv force-pushed the persist-arrays-with-newlines branch from 7b23838 to 3d21d6c Compare May 30, 2016 19:16
@hishamhm hishamhm merged commit b95256c into luarocks:master May 30, 2016
@mpeterv mpeterv deleted the persist-arrays-with-newlines branch May 30, 2016 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants