[#374] Remove minified version of preview_recline.js · ckan/ckan@f35b760 · GitHub

Commit

[#374] Remove minified version of preview_recline.js

Browse files

Should not be in repository.

Loading branch information

johnglover committed Feb 13, 2013

1 parent b4fec95 commit f35b760

ckanext/reclinepreview/theme/public/preview_recline.min.js

This file was deleted.

10 comments on commit `f35b760`

Contributor

vitorbaptista commented on `f35b760` Feb 19, 2013

I agree that it's weird to have these files in the repository, but this commit breaks ckanext.reclinepreview tests when run as

nosetests --ckan --with-pylons=test-core.ini --nologcapture --cover-package=ckanext.reclinepreview ckanext/reclinepreview/tests -x

I'm not sure if the .min.js files should be created automatically when running the tests, or if we should keep them into the repository. What do you think, @tobes?

BTW, jsonpreview and pdfpreview have the same problem.

Contributor

tobes commented on `f35b760` Feb 20, 2013

The idea is that minified versions are created as part of the release process. the best way is via paster front-end-build this updates css/js files and also minimises js/css files.

Part of the reason for this is to reduce the amount of noise in commits

However due to a recent issue vendor files are excluded from this and so the minified files should be added when they are updated. I agree this is slightly confusing

this should be documented somewhere But I don't think it is. This has been discussed in many places to get to the current situation

what is the test problem? can you provide the failing tests output? It sounded on irc like you have an odd issue maybe it's relasted to that. Did you get it fixed.

Contributor

vitorbaptista commented on `f35b760` Feb 20, 2013

Basically, the problem is that we run the tests with debug = false. That means that we're using the minified versions. In https://github.com/okfn/ckan/blob/master/ckanext/jsonpreview/tests/test_preview.py#L83, https://github.com/okfn/ckan/blob/master/ckanext/pdfpreview/tests/test_preview.py#L86, and https://github.com/okfn/ckan/blob/master/ckanext/reclinepreview/tests/test_preview.py#L115, we're checking the presence of a javascript file (non-minified). In my commit efc7472, I've changed that to check for the minified versions.

But, as we don't minify when we run the tests, it only works if the minified version is already created. Which isn't the case anymore for reclinepreview.

I agree that it's better not to leave the minified versions of anything into the repository (third-party libraries don't matter, for me). That they should be created as part of the asset packaging process. But then we should disable it in the tests (debug = true?), and then we come back to your comment at efc7472#commitcomment-2621933.

Maybe the best solution would be to set debug = true, and remove this functionality of creating main.debug.css?

Contributor

tobes commented on `f35b760` Feb 20, 2013

for this issue we could assert
assert 'preview_recline.js' in result.body or 'preview_recline.min.js' in result.body, result.body

that feels like it solves that problem with the test

as far as the minified version is concerned I think this needs a rejig of this extension as recline should be under vendor and the minified file should be generated /updated when recline is or am I confused and this is ckan js in which case that is a different issue as these files will not currently get minified - Maybe that needs adding as a separate issue

Contributor

vitorbaptista commented on `f35b760` Feb 20, 2013

Testing both files feels like a workaround. The issue is that we're asking (through test-core.ini) to the minified versions to be added, but they don't exist. We could either generate them when the tests are ran, or ask for and test the non-minified versions (debug = true).

In this case, I prefer setting debug = true, and removing the main.debug.css code. @johnmartin, which (I guess) is one of the main users of that functionality, seemed happy to remove it as well.

I didn't understand your other comments...

Contributor

tobes commented on `f35b760` Feb 20, 2013

personally I quite like that the tests run with debug=false as we tend to do everything with debug=true in development

the test for both is in my opinion correct behaviour because

a) we will serve non minified files if minified files are not available so we are functioning as expected - and any minified files I might have or not shouldn't really affect the test
b) generating files during testing feels like a bad move. We used to generate minified files on app startup but this had to be removed. If we are generating the minified files during testing I fear it will create noise.
c) really this shouldn't be being tested here at all as we should be testing the fanstatic stuff on it's own
d) all our minification should really happen through the paster front-end-build not as a random side effect of testing

It is a work around because we need it :)

The other comment is the real issue here which is when should the minification be done (via paster in my mind) and currently I believe it is missed

Contributor

vitorbaptista commented on `f35b760` Feb 21, 2013

You're right. In this test, we just want to check that the js file is being included. In an ideal world, we would be able to maybe check that some js methods exist. But here, we assert that by simply grepping for the filename.

It's still weird that, depending on how we run the test, it might have two outcomes (hence the need for the or). We need this workaround because we haven't setup the test case correctly (by whatever means). But I think adding both filenames won't do us any bad. At least I can't see how.

Should we remove all plugin's minified js files, and change the test?

Cheers,
Vítor.

Contributor

tobes commented on `f35b760` Feb 21, 2013

IMHO If a .min.js file exists is about the state of my environment not about the test.

I do not want tests screwing with my environment.

As far as the minified file goes I think it should stay as that is how things currently are github.com/okfn/ckan/tree/master/ckan/public/base/javascript

but we do want to make sure that the paster minify commands catch them which currently they do not.

but seeing how this has been removed from master https://github.com/okfn/ckan/tree/master/ckanext/reclinepreview/theme/public
I'm not quite sure what's going on anymore

Contributor Author

johnglover commented on `f35b760` Feb 21, 2013

I'm not sure what is going on with public/base/javascript. The decision was that the only minified files in the repo should be 3rd party libraries, our code should not exist in the repo in minified form. I suspect that no one has got around to removing the minified files from public/base.

@johnmartin can probably clarify this.

Contributor

tobes commented on `f35b760` Feb 21, 2013

the dev-list is probablythe best place to take this

Please sign in to comment.