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

Closes #1899, Fixed #1898: Adds an easy way to export item names #1900

Merged
merged 1 commit into from Oct 6, 2015
Merged

Closes #1899, Fixed #1898: Adds an easy way to export item names #1900

merged 1 commit into from Oct 6, 2015

Conversation

@thatsIch
Copy link
Member

@thatsIch thatsIch commented Sep 23, 2015

Mostly used for the recipe system, but can also be used for debugging purposes. Debug options needs to be ticked to use the full information gain. Recipes only require the normal localization and the specific name plus metadata.

Shifted the recipes into a recipes folder where the CSV will also reside. This will also elevate the copying of the readme to the user directory since it can reside in the recipes folder.

ss 2015-09-23 at 02 34 31

Fixed a bug where the copier would copy the would also copy empty folders

@@ -1,6 +1,6 @@
/*
* This file is part of Applied Energistics 2.
* Copyright (c) 2013 - 2014, AlgorithmX2, All rights reserved.
* Copyright (c) 2013 - 2015, AlgorithmX2, All rights reserved.

This comment has been minimized.

@yueh

yueh Sep 23, 2015
Member

Maybe just replace it once in every file and then again on every New Year's Day?

This comment has been minimized.

@thatsIch

thatsIch Sep 23, 2015
Author Member

Acutally this happened because I edited the file some time ago sigh

{
Preconditions.checkNotNull( event );

This comment has been minimized.

@yueh

yueh Sep 23, 2015
Member

Missing @Nonnull for the corresponding parameter?

testCompile "junit:junit:4.11"
testCompile "junit:junit:4.12"
testCompile "org.powermock:powermock-module-junit4:1.6.2"
testCompile "org.powermock:powermock-api-easymock:1.6.2"

This comment has been minimized.

@yueh

yueh Sep 23, 2015
Member

Are these dependencies added automatically? There are no unittests for this PR.

This comment has been minimized.

@thatsIch

thatsIch Sep 23, 2015
Author Member

oh.. I knew I forgot something lol.

Wanted to test them out in production.. but in the end Minecraft

@yueh
Copy link
Member

@yueh yueh commented Sep 23, 2015

Seems like there are a couple of bugs with it.

  1. It only extracts the first directory level of the recipes, but no subdirectories even when not empty
  2. The .csv is marked as UTF8, but using ANSI encoding. Like ³ for the spatial cells are not displayed correctly.
  3. It extracts some items multiple times, for example some TD covers or agricraft items. Verbose mode does not add anything to distinguish them. But probably just a minor annoyance.
@thatsIch
Copy link
Member Author

@thatsIch thatsIch commented Sep 23, 2015

  1. works for me. Example? I have thet full recipes/network/**
  2. Fixed internally
  3. There are actually more often in. for example slabs are twice in. It is marked as having sub items but the sub item is empty. So not sure why somebody would implement it like this, but do not think that it is really important for the use case to remove duplicates. (I would also not know a common criteria to sort them out)

There is also no way if you replace your stuff with custom names on runtime like our materials, which are replaced by the recipe parser (probably due to 1.6 to 1.7 code copy)

@yueh
Copy link
Member

@yueh yueh commented Sep 23, 2015

  1. Just purged the full config and it only extracts (aliases|groups|index|oredict).recipe, nothing else and the log did not contain anything useful. .jar does contain all recipes.
  2. Potentially a bit overengineered, but you could use a Set + Tuple with id,meta,name and let it handle any duplicate items before dumping them. But really not that important.

You should also add a debug option to the config. The Forge logger is horrible and will dump any severity level to the log. thus the csv generation is quite spammy.

@thatsIch
Copy link
Member Author

@thatsIch thatsIch commented Sep 23, 2015

  1. Not sure, can not reproduce this on my side. Maybe OS depending? What about outside of dev env? (wants J7)
  2. Would increase complexity by another n or worse. Do not think that it is worth it. This will also not catch people who will register their items in init or post-init.
  3. its set to debug level, so in production should not display these. It also depends on the env you use. MMC e.g. has a finer setting I think.
@yueh
Copy link
Member

@yueh yueh commented Sep 23, 2015

  1. Normal instance on windows.
  2. I had the same topic with Player lately and there is no option to change the forge level. I think the normal log window is set to ERROR, but the fml-client-latest.log will always write everything including DEBUG. Maybe even TRACE
FileUtils.forceMkdir( subDirectory );
this.copyTo( subDirectory, directory + list + "/" );

this.copyTo( identifier, subDirectory, directory + list + File.separatorChar );

This comment has been minimized.

@yueh

yueh Sep 24, 2015
Member

Seems like it does not like File.separatorChar, changing it to a simple / works for me.
As paths in java are platform independent, it will even treat / as a correct separator for windows, but it seems to have an issue with \\ for windows. But only when launching via the vanilla launcher, works perfectly find with eclipse.

@thatsIch
Copy link
Member Author

@thatsIch thatsIch commented Sep 24, 2015

  1. fixed
  2. you can apply the log4j.xml in the worst case. Maybe should use our own logger and not the wrapped one of FML. Could modify a switch upon production via BuildScript or so, Not sure if this is still legit http://forum.feed-the-beast.com/threads/reducing-logging-level.21509/
@yueh
Copy link
Member

@yueh yueh commented Sep 24, 2015

We already have a couple of different config options like the crafting or security audits. So I do not see any reason for not adding a debugLog option. In general it is also a good idea to wrap any call to Log.debug() inside something like Log.isDebugEnabled() as processing the debug data can require some serious processing time. Thus no need to add something to the buildscript, just have it default to false.

@thatsIch
Copy link
Member Author

@thatsIch thatsIch commented Sep 25, 2015

  • Added logging
  • Sorted AEFeature
  • Added debug logging entry
Patterns( Constants.CATEGORY_CRAFTING_FEATURES ),
CraftingCPU( Constants.CATEGORY_CRAFTING_FEATURES ),

ChunkLoggerTrace( Constants.CATEGORY_COMMANDS, false );

This comment has been minimized.

@orod-org

orod-org Sep 25, 2015

MINOR Rename this constant name to match the regular expression '^[A-Z][A-Z0-9]([A-Z0-9]+)_$'. rule

This comment has been minimized.

@thatsIch

thatsIch Sep 25, 2015
Author Member

can not do that, will rewrite the case sens config

This comment has been minimized.

@yueh

yueh Sep 25, 2015
Member

Applying common sense and just ignoring minor legacy stuff is probably ok.

This comment has been minimized.

@thatsIch

thatsIch Sep 27, 2015
Author Member

Upper cased them

@thatsIch
Copy link
Member Author

@thatsIch thatsIch commented Sep 25, 2015

  • Using empty enum for constants

private enum Constants
{
;

This comment has been minimized.

@orod-org

orod-org Sep 25, 2015

MINOR Remove this empty statement. rule

This comment has been minimized.

@thatsIch

thatsIch Sep 27, 2015
Author Member

its not empty you stupid!

This comment has been minimized.

@yueh

yueh Sep 27, 2015
Member

I would not count that the rules are treating this "pattern" as valid.

@thatsIch
Copy link
Member Author

@thatsIch thatsIch commented Sep 25, 2015

  • Upper-cased the config options
…ormation into CSV format

Mostly used for the recipe system, but can also be used for debugging purposes. Debug options needs to be ticked to use the full information gain. Recipes only require the normal localization and the specific name plus metadata.

Shifted the recipes into a recipes folder where the CSV will also reside. This will also elevate the copying of the readme to the user directory since it can reside in the recipes folder.

Fixed a bug where the copier would copy the would also copy empty folders
@orod-org
Copy link

@orod-org orod-org commented Sep 26, 2015

SonarQube analysis reported 5 issues:

  • MAJOR 1 major
  • MINOR 2 minor
  • INFO 2 info

Watch the comments in this conversation to review them.
Note: the following issues could not be reported as comments because they are located on lines that are not displayed in this pull request:

@thatsIch
Copy link
Member Author

@thatsIch thatsIch commented Sep 27, 2015

It is still marking the constructor as an error, though it is not there anymore!

@yueh
Copy link
Member

@yueh yueh commented Sep 27, 2015

I would have to look into which module provides this rule, if it is a bytecode one it might simply not be possible to easily distinguish between a constructor generated by an enum and a normal constructor.

The reports are really just some guidelines and can be ignored with common sense. If i recall correctly, I can mark them as false-positives for the full analysis, which should also prevent them from being reported for an incremental one.

@thatsIch
Copy link
Member Author

@thatsIch thatsIch commented Sep 27, 2015

wth, why would that check bytecode

@yueh
Copy link
Member

@yueh yueh commented Sep 27, 2015

The most obvious reason is simply, that source code for every dependency is not guaranteed. Bytecode being a requirement for compilation guarantees this (except the invalid forge bytecode, which breaks the analysis in this case). Thus resulting in a better analysis which includes every dependency and potential issue arising from them.

The downside is that some stuff like enums or try/catch produce some strange bytecode (like there is no try/catch bytecode at all).

Source code analysers like PMD are also usually built with pattern matching in mind.
So stuff like a line with a single ; (or multiple inside the same line) will already trigger it.

thatsIch added a commit that referenced this pull request Oct 6, 2015
Closes #1899, Fixed #1898: Adds an easy way to export item names
@thatsIch thatsIch merged commit c14bc82 into AppliedEnergistics:master Oct 6, 2015
3 checks passed
3 checks passed
@FireBall1725
default Finished TeamCity Build Applied Energistics :: Pull Requests : Tests passed: 53
Details
@yueh
jenkins Success 53 tests run, 0 skipped, 0 failed.
Details
@orod-org
sonarqube SonarQube reported 5 issues, no critical nor blocker
@thatsIch thatsIch deleted the thatsIch:export-names-to-csv branch Oct 6, 2015
@yueh yueh added this to the rv3 - 1.7.10 milestone Oct 9, 2015
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants