-
Notifications
You must be signed in to change notification settings - Fork 34
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
create resource directory #74
Comments
After my vacation next week :) |
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)); |
should be cm.resourceDir("META-INF/services").addFile(aEntry.getKey (), StandardCharsets.UTF_8, sContent) ; IMO. |
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. |
you can make it without changing the major version (this is the reason of the item 5. ) |
@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. |
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 used JClassAlreadyExistsException but actually there should be specific exceptions, namely ClassNameAlreadyAsFileException and FileNameAlreadyAsClassException
|
Voila. The new exception hierarchy forced me to change the version to 3.4.0 |
@guiguilechat please review. Afterwards I can release |
what about putting the (new) exceptions in a jcodemodel.exceptions package ? |
L264 L270 L548 https://github.com/phax/jcodemodel/blob/master/src/main/java/com/helger/jcodemodel/JResourceDir.java#L67 |
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 try to make code but I'm bad with making pull request and co, so I will instead make a diff. |
Thanks for your review @guiguilechat
|
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.
|
Thanks, but that is not correct. @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");
} |
Those are for classes and dir collisions ; the new test is for file and dir collision. |
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. |
Its also important for tests, because if someone has an issue on a platform, other people may not have that issue on another platform. |
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;
} |
Okay, so I created a lot of additional consistency checks. See the |
|
OK, PR was accepted. I tested if there were different rules for filenames and directories on Linux, but even a Do you want an enum entry in |
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. |
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. |
I'm trying to get rid of those indent issues, sorry for them. |
I fixed the tests - "name" vs. "b" error |
Sorry, closed by accident. |
my b, should make test on tests.
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.
|
Okay, the only thing left open is an additional file system convention - right? |
Will make a new issue for zip/memory file spec. They can be handled later AFAIK. I still think the calls to 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). something like The goal is that all test will pass, whatever is the local environment. |
I need to also cater for backwards compatibility and therefore by default the environment determined from I added a constructor to |
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. 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) |
wrt API simplification :
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??) |
Here is how I would do it.
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() |
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. |
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. |
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;
} |
…s, ignore previous packages. see phax#74
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. |
Since the check on packages name ( #70 ) , it's not possible anymore to create a resource in a resource dir that does not respect a package name.
Example, I can't create a new META-INF/data.txt file, because the META-INF does not respect the package name.
This was explained till #70 (comment) ; I create a separate issue for convenience.
The solution I think would be correct is
public JResourceDir JPackage::asResourceDir(){return m_aOwner.getResourceDir(m_sName);}
public AbstractJResourceFile JPackage::addResourceFile (@Nonnull final AbstractJResourceFile rsrc){return asResourceDir().addResourceFile(rsrc);}
; and same for the other methods.Possibly you can make JPackage extend JResourceDir. Still, overload the methods to call the resource dir when creating resources.
The text was updated successfully, but these errors were encountered: