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

MaxTools #1667

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

MaxTools #1667

wants to merge 7 commits into from

Conversation

kauefelipe
Copy link

Module to show last match U20/U21/Senior

@minj
Copy link
Owner

minj commented Sep 19, 2020

I don't really like that this module is using an external API. This is not a ML model and can be calculated internally

@MaxDareDevil
Copy link

I don't really like that this module is using an external API. This is not a ML model and can be calculated internally

Hi @minj.
The idea to use an API is to avoid duplicate the code for perfect age. The next U21 world cup is really complex and a given player may have two perfect age use. In the future me and @kauefelipe would like to extend the integration between Foxtrick and MaxTools always using APIs. For example allowing at match's order page review a direct result estimation using https://ht-mt.org/match/result-estimation.
We would like to extend the perfect age information adding not only "last match" but also "last complete phase" and informing the amount of training the player would accumulate less than one in real perfect age for the phase.
So if we keep only one code that will be really interesting to avoid rework. This is the reason for the API approach.

@kauefelipe
Copy link
Author

kauefelipe commented Sep 29, 2020

Hi @minj.

Good afternoon. Did you see MaxDareDevil's justification for using an API? You could see the plugin code?
It would be nice to have a solution since the old U20 that is on foxtrick is showing the wrong dates.

@MaxDareDevil
Copy link

MaxDareDevil commented Sep 29, 2020

Hi, I just renamed the account name here to be the same used at HT.

Copy link
Owner

@minj minj left a comment

Choose a reason for hiding this comment

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

On the whole, OK, minor improvements necessary.

Let me know, if you will be addressing my comments yourself.

Alternatively, GitHub now has a feature to allow access for the repo owner (i.e. me) to push into a PR branch on the fork (i.e. your repo)


# MaxTools
module.Maxtools.desc=Show last match U20/U21/Senior
module.Maxtools.RomanNumberEdition.desc=Use Roman numerals in the championship edition
Copy link
Owner

Choose a reason for hiding this comment

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

nit: "for the championship"

module.Maxtools.desc=Show last match U20/U21/Senior
module.Maxtools.RomanNumberEdition.desc=Use Roman numerals in the championship edition
module.Maxtools.YouthPlayers.desc=Show last match U20/U21/Senior in the Youth Player Details page.
module.Maxtools.SeniorPlayers.desc=Show last match U20/U21/Senior in the Senior Player Details page.
Copy link
Owner

Choose a reason for hiding this comment

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

nit: "on the <...> page"

# %3 = phase, e.g. CC
# %4 = WC number, e.g. XIV
# %5 = MaxTools.match
MaxTools.templateShortWithoutTable=%1: %2 - %3 %4 - %5
Copy link
Owner

Choose a reason for hiding this comment

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

nit: EOL missing

//console.log('chegou aqui');
let lang = Foxtrick.Prefs.getString('htLanguage');
//let prefix = 'http://www.fantamondi.it/HTMS/index.php' +
// `?page=htmspoints&lang=${lang}&action=calc`;
Copy link
Owner

Choose a reason for hiding this comment

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

superfluous comments

let lang = Foxtrick.Prefs.getString('htLanguage');
//let prefix = 'http://www.fantamondi.it/HTMS/index.php' +
// `?page=htmspoints&lang=${lang}&action=calc`;
let prefix = 'https://ht-mt.org/nt/perfect-age';
Copy link
Owner

Choose a reason for hiding this comment

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

use HTMT_PATH

else if (isPlayersPage || isYouthPlayersPage)
module.runPlayerList(doc);
else if (isTransferResultsPage)
module.runTransferList(doc);
Copy link
Owner

Choose a reason for hiding this comment

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

method missing

container.appendChild(getLink(player.age.years, player.age.days));
container.appendChild(doc.createTextNode(' '));

champsList.forEach(function (championship) {
Copy link
Owner

Choose a reason for hiding this comment

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

this block also repeats itself => extract and reuse

@@ -225,7 +224,6 @@
<script type="application/x-javascript" src="./shortcuts-and-tweaks/transfer-search-filters.js"></script>
<script type="application/x-javascript" src="./shortcuts-and-tweaks/transfer-search-result-filters.js"></script>
<!-- end categorized modules -->

Copy link
Owner

Choose a reason for hiding this comment

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

this file (and others) should not be edited manually, we have module-update.py for it

@@ -292,6 +292,12 @@ pref("extensions.foxtrick.prefs.module.MatchReportFormat.ShowEventIcons.enabled"
pref("extensions.foxtrick.prefs.module.LiveMatchReportFormat.enabled", true);
pref("extensions.foxtrick.prefs.module.MatchWeather.enabled", true);
pref("extensions.foxtrick.prefs.module.MatchWeather.Irl.enabled", true);
pref("extensions.foxtrick.prefs.module.Maxtools.enabled", true);
pref("extensions.foxtrick.prefs.module.Maxtools.RomanNumberEdition.enabled", true);
pref("extensions.foxtrick.prefs.module.Maxtools.YouthPlayers.enabled", true);
Copy link
Owner

Choose a reason for hiding this comment

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

valid-ids should also be updated, for now

@@ -287,7 +288,8 @@
"*://chpp.hattrick.org/*",
"*://*.foxtrick.org/*",
"http://www.fantamondi.it/*",
"http://pastebin.com/api/*"
"http://pastebin.com/api/*",
"https://ht-mt.org/*"
Copy link
Owner

Choose a reason for hiding this comment

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

have you considered using CORS headers instead?

@minj
Copy link
Owner

minj commented Nov 8, 2020

Apologies for the delay. I was not in a good place mentally for weeks.

@MaxDareDevil
Copy link

Apologies for the delay. I was not in a good place mentally for weeks.
Please do not worry about that. I'm in hurry with RL issues as well. I will talk with @kauefelipe asap.

@minj
Copy link
Owner

minj commented Nov 10, 2020

You could see the plugin code?

@kauefelipe I am not exactly sure, what you've meant here.

Btw, I'll ask for a CHPP feature to confirm this integration

@minj
Copy link
Owner

minj commented Nov 29, 2020

Are you still interested in working on this?

I would specifically like to see these:

  • CORS headers
  • Link to Data/Privacy policy

@kauefelipe
Copy link
Author

Hello minj. I was on vacation last month and I couldn't get anything because I spent a lot of time traveling. I will try to make adjustments in the next few days.

@kauefelipe
Copy link
Author

@minj , I made some of the adjustments you asked for.

I'm new to Git and I'm still learning. I gave you permission in my repository if you want to make any adjustments directly.

@minj
Copy link
Owner

minj commented Nov 30, 2020

Alright I hope to look at it this week but I can't make promises

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