Skip to content
This repository has been archived by the owner on May 24, 2023. It is now read-only.

Compress Option #1

Merged
merged 1 commit into from
Nov 6, 2014
Merged

Compress Option #1

merged 1 commit into from
Nov 6, 2014

Conversation

olirogers
Copy link
Contributor

Hi,

I have been using my own grunt task for a while now that does a similar thing so I merged in the code I used for Javascript, XML and JSON file minification to grunt-openui5.

I have updated the tests to include the new option and some more advanced test data. Let me know if this works for you.

Thanks,

Oli

@matz3 matz3 self-assigned this Oct 14, 2014
@matz3
Copy link
Contributor

matz3 commented Oct 14, 2014

Hi Oli,

thanks for sending this pull-request.

Before your pull-request can be accepted you have to agree to our CLA, see here:
https://github.com/SAP/grunt-openui5/blob/master/CONTRIBUTING.md
and here:
https://github.com/SAP/openui5/blob/master/CONTRIBUTING.md#contributor-license-agreement

Your changes are looking good to me, thanks!
But there are a few problems regarding tabs/spaces and line-endings.

Can you please convert your changes to use tabs and unix line-endings (\n)?

The tests should also be adopted to be independent of line-endings (as you could checkout the files with windows line-endings). This wasn't even the case before your changes, but as you've modified them, I guess it is easier if you change this as well (if you don't mind ;-) otherwise I can do it).
I would suggest to normalize the line-endings using grunt.util.normalizelf.

Thanks,
Matthias

@matz3 matz3 added enhancement New feature or request information required Further information is required labels Oct 14, 2014
@olirogers olirogers force-pushed the compress branch 3 times, most recently from 7a0c2de to 65ea682 Compare November 5, 2014 14:25
@olirogers
Copy link
Contributor Author

Hi Matthias,

Sorry it has taken me some time to get round to fixing this. I think I have managed to do so now though!

I saw you implemented a file comparison test utiliy that I re-used for my purposes also and this seems to make the tests pass on Windows.

I compressed the commits into one and also added the CLA tag as requested.

Let me know if you would like me to make any further changes.

Thanks,

Oli

…xtension.

I hereby declare to agree to the OpenUI5 Individual Contributor License Agreement.
@matz3
Copy link
Contributor

matz3 commented Nov 6, 2014

Thanks Oli! Looks good to me!

There are 2 points that I will take care of after merging your change:

  • The xml compress doesn't seem to handle html tags like <pre> correctly (which could be used in an XMLView). If one has whitespaces only, they are not preserved like it should be. In this rare case, the whole file should just be skipped.
  • Copyright comments in JS files should also be preserved (in the same way like we did it in our internal build, so I will reuse our pattern).

matz3 added a commit that referenced this pull request Nov 6, 2014
@matz3 matz3 merged commit 011d4af into SAP-archive:master Nov 6, 2014
@matz3 matz3 removed the information required Further information is required label Nov 6, 2014
@matz3 matz3 removed their assignment Nov 7, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants