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

Add Java port #27

Merged
merged 1 commit into from
Nov 28, 2015
Merged

Add Java port #27

merged 1 commit into from
Nov 28, 2015

Conversation

pradyunsg
Copy link
Contributor

Part of #23

Any chance of someone running 👀 through this?

@getify
Copy link
Owner

getify commented Jun 15, 2015

Is it appropriate for the tests to be in the same file as the implementation? This seems strange to me.

@pradyunsg
Copy link
Contributor Author

I agree. I'm not very fluent in Java (and I'd presume neither are you). So, any ideas?

@pradyunsg
Copy link
Contributor Author

Also, shouldn't the README.md be modified to add instructions for running tests?

(editted)

@getify
Copy link
Owner

getify commented Jun 15, 2015

shouldn't the README.md be modified?

Yes, see the modifications I made to the README.md in the "javascript", "node" and "php" branches as minimal required updates for each port.

@pradyunsg
Copy link
Contributor Author

But what should the modifications be?

EDIT: If we are moving the tests out, then the change will be different. So, we are moving the tests out, right? Let's address that first.

@getify
Copy link
Owner

getify commented Jun 15, 2015

As indicated in the PORTING.md, item (4), I'd prefer to see something that's consistent with each language's basic conventions. In general, I believe most languages keep implementation and tests separate. I can't think of a strong reason to combine those separate concerns in a single file for any language.

@pradyunsg
Copy link
Contributor Author

I think I can split the Java file. I'll get to it later today or tomorrow.
Just that, what should the package name be? com.getify.json_minify is what I'm thinking...

PS: I should split the python tests as well. Right?

@getify
Copy link
Owner

getify commented Jun 15, 2015

Can we do com.json_minify? I dunno enough about Java to know if that's good or bad, but I don't see why this needs the getify moniker in it. I don't have any other Java software that anyone would need to namespace in.

I should split the python tests as well. Right?

Yes please. :)

@pradyunsg
Copy link
Contributor Author

I guess we can.

Added to tomorrow's TODO list

@pradyunsg pradyunsg changed the title Add 'java' port Add Java port Jul 25, 2015
@nrktkt
Copy link

nrktkt commented Nov 26, 2015

If this is still unresolved... If the java repo is created, I can get this set up with maven and move the tests into appropriate locations with junit. If there is further interest we can get things set up on maven central or in jipack.

@pradyunsg
Copy link
Contributor Author

@kag0

If this is still unresolved...

It is indeed unresolved.

If the java repo is created,

I'm assuming you mean the java branch on this repository, which is created.

I can get this set up with maven and move the tests into appropriate locations with junit.

Please do. I wanted to do this but don't really have that much knowledge (and now, time to gain it) to do this.

If there is further interest we can get things set up on maven central or in jipack.

I'm interested. 😄


Please do read PORTING.md and TESTING.md files. You'll should to make a PR to the java branch in this repository. You could include the work I've done (this PR) or start afresh off the java branch in the repository.

@nrktkt
Copy link

nrktkt commented Nov 27, 2015

Sounds good. Let's merge this PR for now and open issues regarding creating an account and signing keys for maven central, and maybe one for CI/CD.

also, do you mind if I add some inline comments to this PR for improvements? (I'll do them all when I separate the tests if they aren't done before merging the PR)

pradyunsg added a commit that referenced this pull request Nov 28, 2015
@pradyunsg pradyunsg merged commit a5afb72 into getify:java Nov 28, 2015
@pradyunsg
Copy link
Contributor Author

Sounds good. Let's merge this PR for now

Done.

open issues regarding creating an account and signing keys for maven central, and maybe one for CI/CD.

I'd prefer that you open those issues, you are the one with a better idea of what to do.

also, do you mind if I add some inline comments to this PR for improvements? (I'll do them all when I separate the tests if they aren't done before merging the PR)

No I don't. 😄

public class Minify {

public String minify(String jsonString) {
String tokenizer = "\"|(/\\*)|(\\*/)|(//)|\\n|\\r";
Copy link

Choose a reason for hiding this comment

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

Make these strings private static final class variables.

@pradyunsg pradyunsg mentioned this pull request Nov 29, 2015
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