Git Product home page Git Product logo

Comments (45)

phax avatar phax commented on September 16, 2024 1

After my vacation next week :)

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024 1

I used JClassAlreadyExistsException but actually there should be specific exceptions, namely ClassNameAlreadyAsFileException and FileNameAlreadyAsClassException


package com.helger.jcodemodel;

import java.nio.charset.Charset;

import org.junit.Test;

import com.helger.jcodemodel.fmt.JTextFile;

public class JResourceDirTest {

	@Test(expected = IllegalArgumentException.class)
	public void testAbsolutePath() {
		JCodeModel cm = new JCodeModel();
		// this should fail
		cm.resourceDir("/usr");
	}

	@Test(expected = JClassAlreadyExistsException.class)
	public void testClassNameCollision1() throws JClassAlreadyExistsException {
		JCodeModel cm = new JCodeModel();
		cm._package("my")._class(JMod.PUBLIC, "Name");
		// this should fail
		cm.resourceDir("my").addResourceFile(JTextFile.createFully("Name", Charset.defaultCharset(), ""));
	}

	@Test(expected = JClassAlreadyExistsException.class)
	public void testClassNameCollision2() throws JClassAlreadyExistsException {
		JCodeModel cm = new JCodeModel();
		cm.resourceDir("my").addResourceFile(JTextFile.createFully("Name", Charset.defaultCharset(), ""));
		//should fail
		cm._package("my")._class(JMod.PUBLIC, "Name");
	}

}

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024 1

https://github.com/phax/jcodemodel/blob/master/src/main/java/com/helger/jcodemodel/JPackage.java#L264

L264
- JResourceDir aRD = m_aOwner.resourceDir (m_sName.replace ('.', '/'));
+ JResourceDir aRD = m_aOwner.resourceDir (m_sName.replace ('.', JResourceDir.SEPARATOR));

L270
- aRD = m_aOwner.resourceDir (m_sName.toUpperCase (Locale.ROOT).replace ('.', '/'));
+ aRD = m_aOwner.resourceDir (m_sName.toUpperCase (Locale.ROOT).replace ('.', JResourceDir.SEPARATOR));

L548
- return new File (aDir, m_sName.replace ('.', File.separatorChar));
+ return new File (aDir, m_sName.replace ('.', JResourceDir.SEPARATORr));

https://github.com/phax/jcodemodel/blob/master/src/main/java/com/helger/jcodemodel/JResourceDir.java#L67
SEPARATOR = JCFilenameHelper.UNIX_SEPARATOR;
Why not File.separatorChar instead ?

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024 1

https://github.com/phax/jcodemodel/blob/master/src/main/java/com/helger/jcodemodel/JResourceDir.java#L83

I still have issues with that.

the path a////b.txt is correct and semantically the same as a/b.txt ,therefore empty paths are not an issue as long as it's not starting with an empty path (otherwise it's absolute).

Then I see that you make checks for both windows and linux filesystems.
I think it would be better to have an enum "FILESYSTEMCHECKS" that can be {LINUX, WINDOWS, BOTH } and such an attribute in the JCodemodel class. Modifying this attribute woul either test against all the file names in the JCodemodel, or simply crash if any file has already been created.
At first only BOTH should be present ; the filename test should be done in that enum rather than in the jcodemodel (delegation)

I try to make code but I'm bad with making pull request and co, so I will instead make a diff.

from jcodemodel.

phax avatar phax commented on September 16, 2024 1

Thanks for your review @guiguilechat

  • I will not add a new package for exceptions, as this would be even a source incompatible change. This is something for a major release only.

  • I used the the separator more often and added one in JPackage as well

  • I am not using File.separatorChar because it would make fiddling between OS unnecessary difficult, as "/" works on Windows and Linux

  • I am personally still not in favour of allowing multiple consecutive slashes, as it is imho a programming error but for the sake of resiliance... Also certain versions of Windows disallow multiple path separators.

  • And concering the file system checks - I agree. Nevertheless the 2 maps are necessary - one for the real creation and one for the lookup.

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024 1

Those are for classes and dir collisions ; the new test is for file and dir collision.

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024 1

I had a few issues with understanding the basis of file creation, so I need to think about it more.

JCM is used to create a model of the code. This model is then exported from the java representation into files (the write function IIRC). In order to export the model as files (both .java class definitions, and resources), it needs to be sure the targeted environment can accept the file names. It therefore needs to enforce constraints, to warn the user ASAP when it tries to create a file whose name is invalid for the targeted platform.

IMO the "target" should be set at the creation of the JCM, with by default the local platform specs. If I am coding on windows, by default I need to respect windows' naming specs, and if on linux, linux' naming specs.

It's important that I can change the target platform because, the files may be written into a jar file, or on a FTP server, instead of local platform files - Or in memory in my case. This in turn means it's unlikely to change the target after the creation of the first class, and can lead to errors - hence throw an unsupportedoperationException when changing the target of theJCM after class/dir is already created.

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024 1

jcodemodel.util.FileConvention.java

package com.helger.jcodemodel.util;

/**
 * What we are allowed to create as files. Case sensitivity tells if we forbid
 * eg "aa" and "AA" under the same folder, while acceptPath checks that a file
 * does not contain forbidden names.
 */
public interface FileConvention {

	public boolean isCaseSensistive();

	public boolean acceptPath(String path);

}

jcodemodel.util.FileConventions.java

package com.helger.jcodemodel.util;

import java.io.File;
import java.util.function.Predicate;
import java.util.regex.Pattern;

public enum FileConventions implements FileConvention {
	LINUX(true, FileConventions::linuxAllowedChars), WINDOWS(false, FileConventions::windowsAllowedChars)
	;

	public final boolean isCaseSensitive;

	private final Predicate<String>[] checks;

	@SafeVarargs
	private FileConventions(boolean caseSensitive, Predicate<String>... checks) {
		isCaseSensitive = caseSensitive;
		this.checks = checks;
	}

	@Override
	public boolean isCaseSensistive() {
		return isCaseSensitive;
	}

	@Override
	public boolean acceptPath(String path) {
		if (checks != null) {
			for (Predicate<String> p : checks) {
				if (!p.test(path)) {
					return false;
				}
			}
		}
		return true;
	}

	public static FileConventions findPlatform() {
		if (File.separatorChar == '/') {
			return LINUX;
		} else {
			return WINDOWS;
		}
	}

	private static final Pattern windowsForbiddenParts = Pattern.compile(".*[?|*/<>\\\\\\x00:]+.*");

	/**
	 *
	 * @param test
	 *          the part of the directory to test
	 * @return true if the part is allowed for windows
	 */
	public static boolean windowsAllowedChars(String part) {
		return !windowsForbiddenParts.matcher(part).matches();
	}

	private static final Pattern linuxForbiddenParts = Pattern.compile("\\.\\.?");

	/**
	 *
	 * @param test
	 *          the part of the directory to test
	 * @return true if the part is allowed for linux
	 */
	public static boolean linuxAllowedChars(String part) {
		return !linuxForbiddenParts.matcher(part).matches();
	}

}

test resource, package jcodemodel.util.FileConventionsTest

package com.helger.jcodemodel.util;

import org.junit.Assert;
import org.junit.Test;

public class FileConventionsTest {

	@Test
	public void testWindowsInvalidWords() {
		Assert.assertTrue(FileConventions.windowsAllowedChars("aa"));
		Assert.assertFalse(FileConventions.windowsAllowedChars("*aa"));
		Assert.assertFalse(FileConventions.windowsAllowedChars("/aa"));
		Assert.assertFalse(FileConventions.windowsAllowedChars(":aa"));
		Assert.assertFalse(FileConventions.windowsAllowedChars("<aa"));
		Assert.assertFalse(FileConventions.windowsAllowedChars(">aa"));
		Assert.assertFalse(FileConventions.windowsAllowedChars("?aa"));
		Assert.assertFalse(FileConventions.windowsAllowedChars("\\aa"));
		Assert.assertFalse(FileConventions.windowsAllowedChars("|aa"));
		Assert.assertFalse(FileConventions.windowsAllowedChars("a?a"));
		Assert.assertFalse(FileConventions.windowsAllowedChars("aa?"));
		Assert.assertFalse(FileConventions.windowsAllowedChars("aa\0"));
	}

	@Test
	public void testLinuxInvalidWords() {
		Assert.assertTrue(FileConventions.linuxAllowedChars("aa"));
		Assert.assertFalse(FileConventions.linuxAllowedChars("."));
		Assert.assertFalse(FileConventions.linuxAllowedChars(".."));
	}

}

beginning of JCodeModel :

public class JCodeModel implements Serializable
{

	private FileConvention platform = FileConventions.findPlatform();

	/**
	 * @return <code>true</code> if the file system is case sensitive (*x) or
	 *         <code>false</code> if not (e.g. Windows).
	 * @since 3.0.0
	 */
	public boolean isFileSystemCaseSensitive()
	{
		return platform.isCaseSensistive();
	}

	public void setPlatform(FileConvention fc) {
		for (Map<?, ?> c : new Map[] { m_aPackages, m_aResourceDirs }) {
			if (!c.isEmpty()) {
				throw new UnsupportedOperationException("can't change the platform of JCodeModel after creation of the first elements");
			}
		}
		platform = fc;
	}

from jcodemodel.

phax avatar phax commented on September 16, 2024 1

Sorry, closed by accident.
All intermediate paths are stored - that is not yet the case with packages though (v4)

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024

@fbaro

from jcodemodel.

phax avatar phax commented on September 16, 2024

So, what I do to create a "META-INF/services" file, I do the following:

    final String sContent = "bla";
      // Fake directory
      cm.rootPackage ()
        .addResourceFile (JTextFile.createFully ("META-INF/services/" + aEntry.getKey (), StandardCharsets.UTF_8, sContent));

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024

should be

cm.resourceDir("META-INF/services").addFile(aEntry.getKey (), StandardCharsets.UTF_8, sContent) ; IMO.

from jcodemodel.

phax avatar phax commented on September 16, 2024

Agree, would be nicer, but that is kind of a bigger change. The current solution is a valid work around, and I will leave this suggestion open.

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024

you can make it without changing the major version (this is the reason of the item 5. )

from jcodemodel.

phax avatar phax commented on September 16, 2024

@guiguilechat please check if that (3.3.1-SNAPSHOT) looks reasonable to you, or if there further improvements that make sense. Thx again for your input.

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024

https://github.com/phax/jcodemodel/blob/master/src/main/java/com/helger/jcodemodel/JResourceDir.java#L76

// Empty is not allowed
if (sName == null || sName.length () == 0)
return true;

// Java keywords are now allowed
if (!JCFilenameHelper.isValidFilename (sName))
  return true;

No sense for directory. Maybe check that the name does not start with a leading / since all paths are relative, instead ?

Also I think there is a small case issue when you create a resource folder, eg my/folder.txt/ and also create a resource with same name.

Another one, would be to create eg the class my.CreatedClass and at the same time, the file my/CreatedClass .
I think those two could be in the unit tests(did not check in case you thought about them though)

from jcodemodel.

phax avatar phax commented on September 16, 2024

Voila. The new exception hierarchy forced me to change the version to 3.4.0

from jcodemodel.

phax avatar phax commented on September 16, 2024

@guiguilechat please review. Afterwards I can release

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024

what about putting the (new) exceptions in a jcodemodel.exceptions package ?

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024

Here are two tests that check name collision with files and dirs. typically I create "my/name" as both a folder and a file. That should fail, but does not.


	@Test (expected = JResourceAlreadyExistsException.class)
	public void testFileDirNameCollision1() throws JCodeModelException
	{
		final JCodeModel cm = new JCodeModel ();
		cm.resourceDir ("my").addResourceFile (JTextFile.createFully ("name", StandardCharsets.UTF_8, "bla"));
		// should fail
		cm.resourceDir ("my").subDir("name");
	}

	@Test(expected = JResourceAlreadyExistsException.class)
	public void testFileDirNameCollision2() throws JCodeModelException {
		final JCodeModel cm = new JCodeModel();
		cm.resourceDir("my").subDir("name");
		// should fail
		cm.resourceDir("my").addResourceFile(JTextFile.createFully("name", StandardCharsets.UTF_8, "bla"));
		// should fail
	}

from jcodemodel.

phax avatar phax commented on September 16, 2024

Thanks, but that is not correct.
Because for a class the ".java" suffix is appended. And that is already part of the existing JResourceDirTest:

  @Test (expected = JClassAlreadyExistsException.class)
  public void testClassNameCollision1 () throws JCodeModelException
  {
    final JCodeModel cm = new JCodeModel ();
    cm._package ("my")._class (JMod.PUBLIC, "Name");
    // this should fail
    cm.resourceDir ("my").addResourceFile (JTextFile.createFully ("Name.java", StandardCharsets.UTF_8, "bla"));
  }

  @Test (expected = JResourceAlreadyExistsException.class)
  public void testClassNameCollision2 () throws JCodeModelException
  {
    final JCodeModel cm = new JCodeModel ();
    cm.resourceDir ("my").addResourceFile (JTextFile.createFully ("Name.java", StandardCharsets.UTF_8, "bla"));
    // should fail
    cm._package ("my")._class (JMod.PUBLIC, "Name");
  }

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024

Its also important for tests, because if someone has an issue on a platform, other people may not have that issue on another platform.

from jcodemodel.

phax avatar phax commented on September 16, 2024

Okay, so I created a lot of additional consistency checks. See the JResourceDirTest - with filename, directory name and class names. Except for some edge cases where directories have a dot in the name (or if packages are case sensitive) we should be fine.

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024
  • renamed unix-specific format with prefix UNIX_
  • renamed windows-specific format with prefix WINDOWS_ ; also removed _WINDOWS suffix
  • removed test on illegal chars for unix, since eg touch "?" or touch "<" are fine under linux (and result in the file being listed with ls ).
diff --git a/src/main/java/com/helger/jcodemodel/util/JCFilenameHelper.java b/src/main/java/com/helger/jcodemodel/util/JCFilenameHelper.java
index e8f514e..8778c1a 100644
--- a/src/main/java/com/helger/jcodemodel/util/JCFilenameHelper.java
+++ b/src/main/java/com/helger/jcodemodel/util/JCFilenameHelper.java
@@ -65,10 +65,10 @@
   public static final char ILLEGAL_FILENAME_CHAR_REPLACEMENT = '_';
 
   /** Special name of the current path */
-  public static final String PATH_CURRENT = ".";
+  public static final String UNIX_PATH_CURRENT = ".";
 
   /** Special name of the parent path */
-  public static final String PATH_PARENT = "..";
+  public static final String UNIX_PATH_PARENT = "..";
 
   /** The Unix path separator character. */
   public static final char UNIX_SEPARATOR = '/';
@@ -102,15 +102,14 @@
    * Illegal characters in Windows file names.<br>
    * see http://en.wikipedia.org/wiki/Filename
    */
-  private static final char [] ILLEGAL_CHARACTERS_WINDOWS = { 0, '<', '>', '?', '*', ':', '|', '"' };
-  private static final char [] ILLEGAL_CHARACTERS_OTHERS = { 0, '<', '>', '?', '*', '|', '"' };
+  private static final char [] WINDOWS_ILLEGAL_CHARACTERS = { 0, '<', '>', '?', '*', ':', '|', '"' };
 
   /**
    * see http://www.w3.org/TR/widgets/#zip-relative <br>
    * see http://forum.java.sun.com/thread.jspa?threadID=544334&tstart=165<br>
    * see http://en.wikipedia.org/wiki/Filename
    */
-  private static final String [] ILLEGAL_PREFIXES = { "CLOCK$",
+  private static final String [] WINDOWS_ILLEGAL_PREFIXES = { "CLOCK$",
                                                       "CON",
                                                       "PRN",
                                                       "AUX",
@@ -133,7 +132,7 @@
                                                       "LPT8",
                                                       "LPT9" };
 
-  private static final char [] ILLEGAL_SUFFIXES = new char [] { '.', ' ', '\t' };
+  private static final char [] WINDOWS_ILLEGAL_SUFFIXES = new char [] { '.', ' ', '\t' };
 
   static
   {
@@ -226,14 +225,9 @@
       return false;
 
     // Check for reserved directories
-    if (PATH_CURRENT.equals (sFilename) || PATH_PARENT.equals (sFilename))
+    if (UNIX_PATH_CURRENT.equals (sFilename) || UNIX_PATH_PARENT.equals (sFilename))
       return false;
 
-    // Check if file name contains any of the illegal characters
-    for (final char cIllegal : ILLEGAL_CHARACTERS_OTHERS)
-      if (sFilename.indexOf (cIllegal) != -1)
-        return false;
-
     return true;
   }
 
@@ -259,20 +253,20 @@
       return false;
 
     // Check for reserved directories
-    if (PATH_CURRENT.equals (sFilename) || PATH_PARENT.equals (sFilename))
+    if (UNIX_PATH_CURRENT.equals (sFilename) || UNIX_PATH_PARENT.equals (sFilename))
       return false;
 
     // check for illegal last characters
-    if (JCStringHelper.endsWithAny (sFilename, ILLEGAL_SUFFIXES))
+    if (JCStringHelper.endsWithAny (sFilename, WINDOWS_ILLEGAL_SUFFIXES))
       return false;
 
     // Check if file name contains any of the illegal characters
-    for (final char cIllegal : ILLEGAL_CHARACTERS_WINDOWS)
+    for (final char cIllegal : WINDOWS_ILLEGAL_CHARACTERS)
       if (sFilename.indexOf (cIllegal) != -1)
         return false;
 
     // check prefixes directly
-    for (final String sIllegalPrefix : ILLEGAL_PREFIXES)
+    for (final String sIllegalPrefix : WINDOWS_ILLEGAL_PREFIXES)
       if (sFilename.equalsIgnoreCase (sIllegalPrefix))
         return false;
 
@@ -280,7 +274,7 @@
     // Note: we can use the default locale, since all fixed names are pure ANSI
     // names
     final String sUCFilename = sFilename.toUpperCase (Locale.ROOT);
-    for (final String sIllegalPrefix : ILLEGAL_PREFIXES)
+    for (final String sIllegalPrefix : WINDOWS_ILLEGAL_PREFIXES)
       if (sUCFilename.startsWith (sIllegalPrefix + "."))
         return false;
 

from jcodemodel.

phax avatar phax commented on September 16, 2024

Thanks for testing. Basically accepted your proposal.
Do you need more, or shall I start a release build?

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024

I'm testing it now

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024

Just cancel the PR , seems like you already made the patch. Which do you prefer ? a PR or a patch ?

from jcodemodel.

phax avatar phax commented on September 16, 2024

Thanks for your effort. In general I prefer PRs since I'm using Windows without the fancy commandline tools ;-)

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024

kk.

ATM I'm looking at https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT ; But I can't find the format for ZIP file names, which is used for JAR.

4.4.17 file name: (Variable)

   4.4.17.1 The name of the file, with optional relative path.
   The path stored MUST NOT contain a drive or
   device letter, or a leading slash.  All slashes
   MUST be forward slashes '/' as opposed to
   backwards slashes '\' for compatibility with Amiga
   and UNIX file systems etc.  If input came from standard
   input, there is no file name field.  

in-memory filesystem should allow any file name, I guess. That's the file format that is used to make the compilation in memory, so I just write the files into it, and pass them as source for the compiler.


IMO the comment I made #74 (comment) should be used as the javadoc of the interface IFileSystemConvention, and linked in the get and set of the JCodeModel with eg @see utils.IFileSystemConvention

Making a new PR for this one (should be quick)

from jcodemodel.

phax avatar phax commented on September 16, 2024

OK, PR was accepted.

I tested if there were different rules for filenames and directories on Linux, but even a mkdir "*" worked so no need for separate rules.

Do you want an enum entry in EFileSystemConvention that accepts anything and is case sensitive?

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024

Do you want an enum entry in EFileSystemConvention that accepts anything and is case sensitive?

That would be for in-memory but that would be done with the jar spec at the same time and I need to test the limits of in-memory building.

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024

the PR #77 was made for tests of specific issues : the one that does not pass, is if I create a directory ab/b/c and then I try to create a file a/b.

IMO it would be better to store all the intermediate folders, that is if I create the "a/b/c" dir then the three "a", "a/b" and "a/b/c" dirs are created internally.

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024

I'm trying to get rid of those indent issues, sorry for them.

from jcodemodel.

phax avatar phax commented on September 16, 2024

I fixed the tests - "name" vs. "b" error

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024

I fixed the tests - "name" vs. "b" error

my b, should make test on tests.

All intermediate paths are stored - that is not yet the case with packages though (v4)

Well I was gonna make a branch to have the JCodeModel.resourceDir work on the split of the dir path, but it's already the case and I look stupid, sorry.

  JResourceDir aCur = aRootDir;
  for (final String sPart : JCStringHelper.getExplodedArray (JResourceDir.SEPARATOR, sCleanPath))
  {

from jcodemodel.

phax avatar phax commented on September 16, 2024

Okay, the only thing left open is an additional file system convention - right?

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024

Will make a new issue for zip/memory file spec. They can be handled later AFAIK.

I still think the calls to new JCodeModel(EFilespec.LINUX) are very redundant.

I think, either the program should test whether it is in test or not, and then use the test target . But that's bad because that would lead to issues visible in prod, but not visible in tests, for no reason (monkey patch).
IMO it would be better to have the default JCM creation available and modifiable from only one place. I added it in test resource, utils package, but maybe adding it in JCM directly with static method would be better ?

something like public static JCM JCM::unifiedLinux() { return new JCM(EFilespec.LINUX);}
this way if later we have a variable of JCM that depends on the local environment, we can set it in that constructor.

The goal is that all test will pass, whatever is the local environment.

from jcodemodel.

phax avatar phax commented on September 16, 2024

I need to also cater for backwards compatibility and therefore by default the environment determined from JCFilenameHelper.isFileSystemCaseSensitive() should be the default if nothing else is specified.

I added a constructor to JCodeModel to take an IFileSystemConvention but I am not a fan of replicating enum constant names to method names....

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024

okay, I think I missed my explanation.

We have a class whose internal parameter is platform-dependent. This means that tests can potentially work on one platform, and fail on another.
The correct solution for this, is to have one constructor, that is used in tests, and ensures the tests are done against the same platform. That is done eg like you do, by forcing the platform to a unified default value, in the tests. This however can lead to two issues.

First, if another platform-dependent parameter is later created in the class, all calls need to also set that parameter, leading to potential bugs if eg the program is used in another lib and tests are done : if my test only sets the platform, and not the new parameter, then I may use an invalid setting, which could trigger bugs. The solution is to propose a test-environment purpose method, so that any test that uses that method will be given a tested platform parameters.

Second, it means that you need to change the calls every where in case you change the default test platform, which can also be a source of bugs.

Therefore, I recommend to have a static method, eg in JCM, that provides a unified test-environment JCM, and that is easy to modify. I used "linux" before, but you actually can use something like "createTestJCM" or "createUnified" (with correct javadoc that this method is designed for unified tests, and specification in JCM::new that this method should be used for tests)

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024

wrt API simplification :
IMO (but just my opinion) a better API would be

  • setX(X new) returns the old X (instead of this). typically checkX(new); old = my_x; my_x=new; return old;
  • withX(X new) returns this after calling setX(new). typically setX(new); return this;

This way you can set the X out of the constructor. It's better because constructors with a parameter are often implied to have those parameters final. Therefore, I would :advocado: for an empty constructor. Also if we have a constructor for every parameter, it can get heavy fast.

also setting to null would set to default one ? (not sure at all??)

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024

Here is how I would do it.

	/**
	 * Default constructor using the system default file system convention.
	 */
	public JCodeModel ()
	{
		try {
			setFileSystemConvention(EFileSystemConvention.DEFAULT);
		} catch (JCodeModelException e) {
			// should not happen
			throw new UnsupportedOperationException("catch this", e);
		}
	}

	/**
	 * @return The file system convention to be used. Never <code>null</code>.
	 * @since 3.4.0
	 * @see IFileSystemConvention
	 */
	@Nonnull
	public final IFileSystemConvention getFileSystemConvention ()
	{
		return m_aFSConvention;
	}

	/**
	 * Set the target file system convention to be used. This method MUST be
	 * called BEFORE the first package or resource directory is created. Later
	 * calls result in an exception.
	 *
	 * @param aFSConvention
	 *        The file system convention to be used. May not be <code>null</code>.
	 * @return this for chaining
	 * @throws JCodeModelException
	 *         if a package or a resource directory is already present.
	 * @see IFileSystemConvention
	 * @since 3.4.0
	 */
	@Nonnull
	public final IFileSystemConvention setFileSystemConvention(@Nonnull final IFileSystemConvention aFSConvention)
			throws JCodeModelException
	{
		IFileSystemConvention old = m_aFSConvention;
		JCValueEnforcer.notNull (aFSConvention, "FSConvention");
		if (!m_aPackages.isEmpty () || !m_aResourceDirs.isEmpty ()) {
			throw new JCodeModelException ("The FileSystem convention cannot be changed if a package or a resource directory already exists.");
		}
		m_aFSConvention = aFSConvention;
		return old;
	}

	@Nonnull
	public final JCodeModel withFileSystemConvention(@Nonnull final IFileSystemConvention aFSConvention)
			throws JCodeModelException {
		setFileSystemConvention(aFSConvention);
		return this;
	}

	public static JCodeModel createUnified() {
		try {
			return new JCodeModel().withFileSystemConvention(EFileSystemConvention.LINUX);
		} catch (JCodeModelException e) {
			// should not happen
			throw new UnsupportedOperationException("catch this", e);
		}
	}

Then I would temporary set the constructor to private, in order to have compile errors in the tests, and there replace all the new JCM() by JCM.createUnified()

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024

sorry for bad indentation, I made my git replace all double-space with tab on pull, and replace all tabs with double-space on push ( it's actually quick and easy ) but eclipse keeps on removing double tabs even when I asked the autoformat on save to only format modified text.

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024

BTW I'm working on allowing to change the platform dynamically.

The issue is when going from linux to windows, and then back to linux, it can change the casing of a file/dir . typically when I create the dir "a" for linux platform, then go to windows, it should replace it with "A" ; and when going back to linux it will not change it again.
A solution is to accept the new platform only when it has same case sensitivity.

from jcodemodel.

guiguilechat avatar guiguilechat commented on September 16, 2024

example

	/**
	 * Set the target file system convention to be used. This method is better
	 * called BEFORE the first package or resource directory is created. Later
	 * calls result in an exception, if the new convention tries to change case
	 * sensitivy or prevent the creation of resources names that are already used.
	 *
	 * @param aFSConvention
	 *          The file system convention to be used. May not be
	 *          <code>null</code>.
	 * @return this for chaining
	 * @throws JCodeModelException
	 *           if a package or a resource directory is already present.
	 * @see IFileSystemConvention
	 * @since 3.4.0
	 */
	@Nonnull
	public final IFileSystemConvention setFileSystemConvention(@Nonnull final IFileSystemConvention aFSConvention)
			throws JCodeModelException
	{
		IFileSystemConvention old = m_aFSConvention;
		JCValueEnforcer.notNull (aFSConvention, "FSConvention");
		if (!m_aPackages.isEmpty () || !m_aResourceDirs.isEmpty ()) {
			// test null in case we set the platform from the constructor
			if (m_aFSConvention != null && m_aFSConvention.isCaseSensistive() != aFSConvention.isCaseSensistive()) {
				throw new JCodeModelException ("The FileSystem convention cannot be changed case sensitivity if a package or a resource directory already exists.");
			}
			for (FSName name : m_aResourceDirs.keySet()) {
				String sName = name.getName();

				// copy from JresourceDir. should be mutualized ?

				// An empty directory name is okay
				if (sName.length() > 0) {
					for (final String sPart : JCStringHelper.getExplodedArray(JResourceDir.SEPARATOR, sName)) {
						if (!aFSConvention.isValidDirectoryName(sPart)) {
							throw new IllegalArgumentException("Resource directory name '" + sName
									+ "' contains the the invalid part '" + sPart + "' according to the current file system conventions");
						}
					}
				}
			}

			// nothing to do with packages, file names convention is not relevant to
			// them.
		}
		m_aFSConvention = aFSConvention;
		return old;
	}

from jcodemodel.

stale avatar stale commented on September 16, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

from jcodemodel.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.