Skip to content
This repository has been archived by the owner on Feb 23, 2022. It is now read-only.

Adds support for --allow-duplicate-declarations flag #79

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Adds support for --allow-duplicate-declarations flag #79

wants to merge 6 commits into from

Conversation

BradMclain
Copy link

Adds support for flag --allow-duplicate-declarations.

Please note was originally implmented by @amralassal here #75 however I needed this feature ASAP so implemented myself.

@BradMclain BradMclain changed the title Allow duplicate declarations Adds support for --allow-duplicate-declarations flag Mar 17, 2016
@iflan
Copy link
Member

iflan commented Mar 17, 2016

I'm going to pull this change internally to verify that it doesn't break anything.

@iflan
Copy link
Member

iflan commented Mar 17, 2016

So, I pulled this change to get it reviewed internally and there were some concerns:

  • It looks like that with this change, setting eliminateDeadStyles to false no longer allows duplicate declarations unless you turn that on as well. This may break existing users.
  • It's unclear whether the combination of eliminateDeadStyles and allowDuplicateDeclarations actually works as intended.

Unfortunately, I don't have time to address these myself if you want it done right away. But if you could, I'd be happy to review the changes.

I think the first thing to do is to create a test for setting eliminateDeadStyles and allowDuplicateDeclarations both to true. You could add it to ClosureCommandLineCompilerTest.java.

If that works, then I'd like the logic for eliminateDeadStyles to default to disallowing duplicate declarations unless allowDuplicateDeclarations is on. Could you do that?

Thanks a bunch!

@BradMclain
Copy link
Author

I will try and make these changes when I get a chance.

@pspeter3
Copy link

I'd like this as well, see #89, is there anyway that I can help to finish this work?

@BradMclain
Copy link
Author

I can add you to our repo and you can pick up from where I finished. I have been too busy to do anymore on this recently.

Alternatively if you just need it working for a project you can checkout the branch and compile your own version of the compiler to use.

https://github.com/anomaly/closure-stylesheets

@pspeter3
Copy link

I will likely create a fork for Asana and do the work necessary there. I just want to make sure that this will get merged if done. Thanks for the advice!

@BradMclain
Copy link
Author

BradMclain commented Oct 4, 2017

Since there has been no further progress on #90 (presumably due to missing CLA) I went ahead and added the test from that PR into this one.

@iflan Do the issues you outlined previously still need addressing?

@jorgedavila25
Copy link

Will this get merged at some point? Thanks for the work @BradMclain

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants